From 1820630392fb330af10ccae85f751863380cdaea Mon Sep 17 00:00:00 2001 From: Erica Fischer Date: Thu, 6 Aug 2026 16:47:45 -0700 Subject: [PATCH] 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 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 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 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 Claude-Session: https://claude.ai/code/session_011wLk2itWETPBAS9a9yE8zu --------- Co-authored-by: Claude --- Makefile | 40 ++++++++++++++++++++++++++++++++++++++-- main.cpp | 31 +++++++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 58c7bb1f..df38c0d6 100644 --- a/Makefile +++ b/Makefile @@ -133,7 +133,7 @@ indent: TESTS = $(wildcard tests/*/out/*.json) SPACE = $(NULL) $(NULL) -test: tippecanoe tippecanoe-decode $(addsuffix .check,$(TESTS)) raw-tiles-test parallel-test pbf-test join-test enumerate-test decode-test join-filter-test unit json-tool-test allow-existing-test csv-test layer-json-test pmtiles-test decode-pmtiles-test overzoom-test flatgeobuf-test +test: tippecanoe tippecanoe-decode $(addsuffix .check,$(TESTS)) raw-tiles-test parallel-test radix-sort-test pbf-test join-test enumerate-test decode-test join-filter-test unit json-tool-test allow-existing-test csv-test layer-json-test pmtiles-test decode-pmtiles-test overzoom-test flatgeobuf-test ./unit suffixes = json json.gz @@ -168,7 +168,7 @@ nogeobuf = tests/overflow/out/-z0.json $(wildcard tests/stringid/out/*.json) geobuf-test: tippecanoe-json-tool $(addsuffix .checkbuf,$(filter-out $(nogeobuf),$(TESTS))) # For quicker address sanitizer build, hope that regular JSON parsing is tested enough by parallel and join tests -fewer-tests: tippecanoe tippecanoe-decode geobuf-test raw-tiles-test parallel-test pbf-test join-test enumerate-test decode-test join-filter-test unit +fewer-tests: tippecanoe tippecanoe-decode geobuf-test raw-tiles-test parallel-test radix-sort-test pbf-test join-test enumerate-test decode-test join-filter-test unit # XXX Use proper makefile rules instead of a for loop %.json.checkbuf: @@ -179,6 +179,42 @@ fewer-tests: tippecanoe tippecanoe-decode geobuf-test raw-tiles-test parallel-te cmp $@.out $(patsubst %.checkbuf,%,$@) rm $@.out $@.mbtiles +# The result of the sort must not depend on how the sort was performed, so instead of +# checking the sorted output against an expected copy of it, check that sorting by radix +# produces the same tiles as sorting in memory. --prefer-radix-sort lowers the memory +# limit to 8K, which radix() then halves again, so the radix subdivision has to recurse +# until each bucket is under 4K. How deeply that recurses depends on how many files the +# machine will let us open at once, but the sorted result is the same either way, so +# this comparison doesn't depend on that. +# +# What sends the sort down its rarely-taken paths is the shape of the input rather than +# the size of it: a feature whose geometry alone is bigger than the memory limit is +# sorted as a bucket of its own, and features that share a long run of leading index +# bits have to be subdivided until there are no bits left. The first two inputs are +# each one of those on purpose -- several separated features too big to sort in memory, +# and many features at one location -- and the rest are for breadth. +radix-sort-test: tippecanoe tippecanoe-decode + mkdir -p tests/radix-sort + perl -e 'for ($$f = 0; $$f < 8; $$f++) { print "{ \"type\": \"Feature\", \"properties\": { \"f\": $$f }, \"geometry\": { \"type\": \"LineString\", \"coordinates\": ["; for ($$i = 0; $$i < 2000; $$i++) { print "," unless $$i == 0; printf "[%f,%f]", $$f * 40 - 175 + $$i * 0.001, $$i % 2 * 0.5 - 20; } print "] } }\n"; }' > tests/radix-sort/bigfeatures.json + perl -e 'for ($$i = 0; $$i < 500; $$i++) { print "{ \"type\": \"Feature\", \"properties\": { \"i\": $$i }, \"geometry\": { \"type\": \"Point\", \"coordinates\": [ 17, 42 ] } }\n"; }' > tests/radix-sort/onelocation.json + $(MAKE) radix-sort-compare RADIXIN="tests/radix-sort/bigfeatures.json" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/radix-sort/onelocation.json" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/feature-filter/in.json" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/ne_110m_ocean/in.json" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/border/in.json" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/loop/in.json" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/tl_2022_11_tract/in.json.gz" RADIXARGS="-z4" + $(MAKE) radix-sort-compare RADIXIN="tests/epsg-3857/in.json" RADIXARGS="-z4 -sEPSG:3857" + rm -r tests/radix-sort + +radix-sort-compare: + ./tippecanoe -q -f $(RADIXARGS) -o tests/radix-sort/memory.mbtiles $(RADIXIN) + ./tippecanoe -q -f $(RADIXARGS) -aR -o tests/radix-sort/radix.mbtiles $(RADIXIN) + ./tippecanoe-decode -x generator -x generator_options -x name -x description tests/radix-sort/memory.mbtiles > tests/radix-sort/memory.json + ./tippecanoe-decode -x generator -x generator_options -x name -x description tests/radix-sort/radix.mbtiles > tests/radix-sort/radix.json + cmp tests/radix-sort/memory.json tests/radix-sort/radix.json + rm tests/radix-sort/memory.mbtiles tests/radix-sort/radix.mbtiles tests/radix-sort/memory.json tests/radix-sort/radix.json + parallel-test: $(eval SHELL:=$(ADVSHELL)) mkdir -p tests/parallel perl -e 'for ($$i = 0; $$i < 20; $$i++) { $$lon = rand(360) - 180; $$lat = rand(180) - 90; $$k = rand(1); $$v = rand(1); print "{ \"type\": \"Feature\", \"properties\": { \"yes\": \"no\", \"who\": 1, \"$$k\": \"$$v\" }, \"geometry\": { \"type\": \"Point\", \"coordinates\": [ $$lon, $$lat ] } }\n"; }' > tests/parallel/in1.json diff --git a/main.cpp b/main.cpp index b7813995..5842f988 100644 --- a/main.cpp +++ b/main.cpp @@ -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 *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 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");