mirror of
https://github.com/felt/tippecanoe.git
synced 2026-10-02 08:25:40 +02:00
Fix the radix sort, and check that it agrees with the in-memory sort (#404)
* Don't write an extra byte when the radix sort writes a bucket directly
radix1() writes out a sorted bucket in two places. merge() writes all but
the last byte of each serialized feature and then appends the byte for the
feature minzoom, since the minzoom is the last byte of the feature. The path
taken when a bucket holds only one feature, or when the recursion has
consumed every bit of the index, instead writes the feature's whole
serialized length and then appends another minzoom byte, which is one byte
more than the feature's length prefix says it is. Everything read from the
geometry afterward is then misaligned by a byte.
--prefer-radix-sort lowers the memory limit to 8K so that this code gets
exercised, and any bucket that has to be written directly is enough to
desynchronize the stream, so it fails on several of the existing test
inputs:
$ ./tippecanoe -q -f -o out.mbtiles -z4 -aR tests/ne_110m_ocean/in.json
wrong length decoding feature: used 10, len is 33
Write one byte less here too, as merge() does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011wLk2itWETPBAS9a9yE8zu
* Keep the radix sort from recursing forever when it runs out of files
radix1() subdivides a bucket by the next splitbits bits of the index, and
stops recursing once prefix + splitbits reaches the width of the index.
The number of buckets comes from the number of files still available, which
shrinks at every level, so deep enough recursion reaches availfiles / 4 == 1
and therefore splitbits == 0. At that point the recursion consumes no bits
of the index and availfiles stops shrinking, so prefix never advances and
the recursion has no way to terminate.
A splitbits of 0 also makes the shift that chooses a feature's bucket a
shift by the full width of the index, which is undefined. In practice it
leaves the shift count masked to zero, so the bucket number is the whole
index rather than 0, and writing to that bucket runs off the end of the
arrays of open files.
Require at least two buckets so that each subdivision always consumes at
least one bit of the index and the shift is always in range, and don't
recurse at all when the next level would not have enough files to split
with: sort that bucket in memory instead, even though it is larger than
the memory limit asked for, since that is the only way left to get it
sorted.
--prefer-radix-sort, which lowers the memory limit to 8K so that this code
gets exercised, segfaults on tests/feature-filter/in.json without this:
$ ./tippecanoe -q -f -o out.mbtiles -z0 -aR tests/feature-filter/in.json
Segmentation fault
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011wLk2itWETPBAS9a9yE8zu
* Check that the radix sort and the in-memory sort agree
The result of a sort shouldn't depend on how the sort was performed, so
rather than checking the sorted output against a committed copy of it,
check that --prefer-radix-sort, which lowers the memory limit to 8K to
force the radix subdivision to recurse, produces the same tiles as sorting
in memory. Nothing new has to be kept up to date, and the comparison holds
regardless of how deeply the subdivision recurses on a given machine, which
depends on how many files it will let us open at once.
What sends the sort down the paths that are otherwise almost never taken is
the shape of the input rather than the size of it, so two small inputs are
generated for the purpose: several well-separated features that are each
too big to sort in memory, which are each written out as a bucket of their
own, and many features at one location, which have to be subdivided until
there are no index bits left. Between them and tests/feature-filter, all
three of radix1()'s branches are covered, including sorting in memory
because there are no files left to subdivide with.
Both of these inputs fail without the two preceding commits, and every
input here failed before them.
The whole target runs in about ten seconds.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011wLk2itWETPBAS9a9yE8zu
* Record what the shift by the full index width actually did
Say in the comment that masking the shift count to zero makes the bucket
number come out as the whole shifted index, so the writes go somewhere
past the end of the arrays of buckets, rather than only that the shift is
undefined.
Also correct the note on the test: --prefer-radix-sort sets the memory
limit to 8K, but radix() halves it again, so the subdivision is working
against 4K.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011wLk2itWETPBAS9a9yE8zu
---------
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
905fe84459
commit
1820630392
@@ -743,8 +743,20 @@ void start_parsing(int fd, STREAM *fp, long long offset, long long len, std::ato
|
||||
}
|
||||
|
||||
void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int splits, long long mem, const char *tmpdir, long long *availfiles, FILE *geomfile, FILE *indexfile, std::atomic<long long> *geompos_out, long long *progress, long long *progress_max, long long *progress_reported, int maxzoom, int basezoom, double droprate, double gamma, struct drop_state *ds) {
|
||||
// Arranged as bits to facilitate subdividing again if a subdivided file is still huge
|
||||
int splitbits = log(splits) / log(2);
|
||||
// Arranged as bits to facilitate subdividing again if a subdivided file is still huge.
|
||||
//
|
||||
// There must be at least two buckets. With only one, each subdivision would
|
||||
// consume no bits of the index, so it would never reach the maximum prefix
|
||||
// that stops the recursion, and the shift that chooses a feature's bucket
|
||||
// below would be by the full width of the index. That shift is undefined,
|
||||
// and what it does in practice is to mask the shift count down to zero, so
|
||||
// the bucket number comes out as the whole shifted index instead of as 0
|
||||
// and the writes are made through whatever is found beyond the end of the
|
||||
// arrays of buckets.
|
||||
int splitbits = 1;
|
||||
if (splits > 1) {
|
||||
splitbits = log(splits) / log(2);
|
||||
}
|
||||
splits = 1 << splitbits;
|
||||
|
||||
FILE *geomfiles[splits];
|
||||
@@ -891,7 +903,14 @@ void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int split
|
||||
}
|
||||
|
||||
if (indexst.st_size > 0) {
|
||||
if (indexst.st_size + geomst.st_size < mem) {
|
||||
// Subdividing again would only make progress if the next level could
|
||||
// split into at least two buckets, which it can't if there are no longer
|
||||
// enough files left to do it with. In that case, sort in memory instead,
|
||||
// even though this is more memory than we wanted to use at once, since
|
||||
// it is the only way left to get this bucket sorted.
|
||||
bool can_subdivide = *availfiles / 4 > 1;
|
||||
|
||||
if (indexst.st_size + geomst.st_size < mem || !can_subdivide) {
|
||||
std::atomic<long long> indexpos(indexst.st_size);
|
||||
int bytes = sizeof(struct index);
|
||||
|
||||
@@ -994,7 +1013,11 @@ void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int split
|
||||
struct index ix = indexmap[a];
|
||||
long long pos = *geompos_out;
|
||||
|
||||
fwrite_check(geommap + ix.start, ix.end - ix.start, 1, geomfile, geompos_out, "geom");
|
||||
// MAGIC: This knows that the feature minzoom is the last byte of the serialized feature
|
||||
// and is writing one byte less and then adding the byte for the minzoom,
|
||||
// the same as merge() does.
|
||||
|
||||
fwrite_check(geommap + ix.start, 1, ix.end - ix.start - 1, geomfile, geompos_out, "geom");
|
||||
int feature_minzoom = calc_feature_minzoom(&ix, ds, maxzoom, gamma);
|
||||
serialize_byte(geomfile, feature_minzoom, geompos_out, "merge geometry");
|
||||
|
||||
|
||||
Reference in New Issue
Block a user