Convert jsonpull to C++ with shared_ptr and std::vector/std::string - #388
Open
e-n-f wants to merge 13 commits into
Open
Convert jsonpull to C++ with shared_ptr and std::vector/std::string#388e-n-f wants to merge 13 commits into
e-n-f wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the in-repo jsonpull JSON parser from C/manual allocation to C++ using std::shared_ptr ownership and STL containers, and updates the rest of the codebase to use the new json_object_ptr / json_pull_ptr APIs.
Changes:
- Replaces
jsonpull.cwithjsonpull.cppand updatesjsonpull.hto expose C++ types (std::shared_ptr,std::vector,std::string) and accessors. - Updates GeoJSON parsing/serialization, filtering/evaluation, and metadata handling to use
json_object_ptrandjson_stringify()returningstd::string. - Propagates new pointer types through tile generation/join/overzoom tooling and related utilities.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tile.hpp | Updates traverse_zooms() filter parameter to json_object_ptr. |
| tile.cpp | Updates filter/parser pointer types (json_object_ptr, json_pull_ptr) through tiling pipeline. |
| tile-join.cpp | Updates filter plumbing and JSON traversal to json_object_ptr and STL-backed accessors. |
| read_json.hpp | Updates function signatures to accept json_object_ptr / json_pull_ptr. |
| read_json.cpp | Switches JSON stringify usage to std::string and updates JSON navigation to new accessors. |
| pmtiles_file.cpp | Updates metadata JSON parsing/serialization to new JSON APIs and string return types. |
| plugin.hpp | Updates parse_feature() signature to accept json_pull_ptr. |
| plugin.cpp | Updates filter output parsing to json_object_ptr / json_pull_ptr and removes manual frees. |
| overzoom.cpp | Updates filter pointer type to json_object_ptr. |
| Makefile | Updates indent target to format jsonpull/jsonpull.cpp instead of jsonpull/*.[ch]. |
| main.cpp | Updates stream JSON begin API and input parsing call sites to json_pull_ptr / json_object_ptr. |
| jsontool.cpp | Updates JSON plumbing to shared_ptr, STL, and json_stringify() returning std::string. |
| jsonpull/jsonpull.h | Introduces C++ shared_ptr-based API, STL-backed value storage, and accessor methods. |
| jsonpull/jsonpull.cpp | New C++ implementation of jsonpull parser/stringifier using shared ownership. |
| jsonpull/jsonpull.c | Removes the legacy C implementation. |
| geometry.hpp | Updates overzoom() signatures to accept json_object_ptr filter. |
| geojson.hpp | Updates JSON parse argument types and map-backed parser API types. |
| geojson.cpp | Updates GeoJSON serialization/parsing to new JSON APIs and shared ownership. |
| geojson-loop.hpp | Updates json_feature_action interface and parse entrypoint to shared_ptr types. |
| geojson-loop.cpp | Updates feature/geometry detection loop to new JSON APIs and std::string stringify. |
| geobuf.cpp | Updates parsing of embedded tippecanoe JSON to shared_ptr types. |
| evaluator.hpp | Updates filter/evaluation API to accept json_object_ptr and returns shared_ptr filters. |
| evaluator.cpp | Updates filter evaluation and error reporting to new JSON APIs and std::string stringify. |
| dirtiles.cpp | Updates directory metadata JSON parsing to shared_ptr types and STL traversal. |
| clip.cpp | Updates overzoom() signatures to accept json_object_ptr filter. |
| attribute.cpp | Updates JSON parsing for -E accumulation config to shared_ptr types and STL traversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+554
to
+556
| } else if (ch >= 0xdc00 && c <= 0xdfff) { | ||
| if (surrogate >= 0) { | ||
| long c1 = surrogate - 0xd800; |
Replace the manual malloc/realloc/free memory management in jsonpull with std::shared_ptr ownership. Each json_object now owns its children through std::vector<json_object_ptr>; raw back-pointers to parent and parser remain valid by structural invariant and are cleared on json_disconnect so detached subtrees can outlive their parser. Strings become std::string, child arrays become std::vector, and the old union becomes a struct so non-trivial members can coexist while preserving the existing o->value.xxx access paths. The old jsonpull.c is replaced by jsonpull.cpp, json_stringify now returns std::string, and all callers across tippecanoe, tile-join, tippecanoe-decode, tippecanoe-json-tool, tippecanoe-overzoom and the unit tests are updated to use json_object_ptr / json_pull_ptr. Co-authored-by: Cursor <cursoragent@cursor.com>
The previous "every member in a struct" layout cost 168 bytes per json_object, even for JSON_NULL / JSON_TRUE / JSON_FALSE nodes that have no payload. Splitting json_object into a small base class plus json_number / json_string / json_array / json_hash subclasses brings each instance down to just the size of its actual contents: json_object (base, TRUE / FALSE / NULL) 24 bytes json_number 48 bytes json_string 48 bytes json_array (empty) 48 bytes json_hash (empty) 72 bytes Other size wins along the way: * Drop enable_shared_from_this<json_object> (its embedded weak_ptr was 16 bytes per node). json_pull now keeps an explicit container_stack and the parser no longer needs to resurrect a shared_ptr from a raw `parent` walk. * Remove the unused `refcon` slot from the string variant. * No virtual destructor: shared_ptr keeps the deleter from the original std::make_shared<json_xxx> call, so destroying a shared_ptr<json_object> still runs the right subclass dtor. The base class exposes type-tagged accessors (o->string(), o->number(), o->array(), o->keys(), o->values(), o->large_signed(), o->large_unsigned()) that assert the type matches and downcast to the appropriate subclass storage. All call sites were swept from the old `o->value.X.Y` field paths to these accessors. A raw-pointer overload of json_hash_get() replaces the few external uses of shared_from_this() that survived in geojson-loop.cpp. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the parallel std::vector<json_object_ptr> keys / values on
json_hash with a single std::vector<json_entry>, where json_entry is
a small {key, value} aggregate. This still preserves insertion order
(the property the parallel vectors were providing) but removes the
"keep two vectors in lockstep" pattern, and call sites can now use
range-for with structured bindings:
for (auto &[k, v] : o->entries()) { ... }
Side effects:
* sizeof(json_hash) drops from 72 to 48 bytes (one fewer vector
header), matching json_array.
* The keys() and values() accessors on json_object are replaced by a
single entries() accessor returning std::vector<json_entry>&.
* All call sites were swept from the old paired-index pattern
(`o->keys()[i]` / `o->values()[i]`) to entry-based access. Where the
original pattern relied on `nprop = 0` to short-circuit iteration on
a null or non-hash `properties`, the rewrite now guards the loop
explicitly with `if (o->type == JSON_HASH)` so that calling
entries() doesn't trip the asserting downcast.
Co-authored-by: Cursor <cursoragent@cursor.com>
`expect` was only meaningful while the parser was building a container,
and only ever read or written from jsonpull.cpp itself; once parsing
finished it was dead weight on every JSON_ARRAY and JSON_HASH (and
present-but-unused on every primitive too). Move it into the parser's
container stack, alongside the shared_ptr to the container it pertains
to:
struct json_pull::parse_frame {
json_object_ptr container;
json_type expect;
};
std::vector<parse_frame> container_stack;
The base class now only carries data-model state (parent, parser, type).
No external caller depended on `expect`, so no sweep was needed outside
jsonpull.cpp.
This change does not, in itself, shrink any json_object: the 4-byte
`expect` field used to live at offset 20 inside the base, where it was
already being eaten by alignment padding for the 8-byte-aligned first
member of every subclass (std::string, std::vector, double). The win is
in the data model, not the byte count -- the 4-byte hole is still
there, but it is now available for a future subclass whose first member
is small enough to slot into it.
Co-authored-by: Cursor <cursoragent@cursor.com>
json_number used to carry three parallel 8-byte fields (a double plus
both a 64-bit unsigned and a 64-bit signed slot for the large-integer
cases) even though at most one of the integer slots is ever the
canonical value for any given number. Collapse them into a
discriminated union:
enum repr_t { REPR_DOUBLE, REPR_LARGE_UNSIGNED, REPR_LARGE_SIGNED };
repr_t repr;
union { double d; unsigned long long u; long long s; } value;
Callers keep the same read API: number() returns the appropriate
double, large_unsigned() returns the ull (or 0 if not currently stored
that way), large_signed() likewise. Writes go through new set_number /
set_large_unsigned / set_large_signed methods that keep the
discriminator and the union value in sync.
This was prompted by an observation that moving json_type to the end
of the object should shrink things via tail-padding reuse. Empirically
the type-at-end rearrangement saves nothing on its own (every
subclass payload is 8-byte aligned so it can't slot into the 4-byte
tail), but the discriminated-number redesign hits the same idea from
a different direction: adding the 4-byte `repr` to json_number makes
the class non-standard-layout, which lets the Itanium ABI pack `repr`
into the base's 4-byte tail padding at offset 20. The union value
then starts at the natural offset 24, and json_number ends at offset
32 -- a 33% reduction.
Per-node sizes:
json_object (TRUE/FALSE/NULL) 24 bytes
json_number 32 bytes (was 48)
json_string 48 bytes
json_array 48 bytes
json_hash 48 bytes
Numbers dominate real GeoJSON (every coordinate is one), so the net
memory win on a typical parse is substantial.
Co-authored-by: Cursor <cursoragent@cursor.com>
- jsontool.cpp `out()`: route JSON_NUMBER (and anything else non-string)
through `json_stringify` instead of `o->string()`, which now asserts
on a non-string type and would crash `--extract` on numeric attributes.
- geojson.{hpp,cpp} `json_end_map`: take `json_pull_ptr` by reference so
the caller's shared_ptr is released, null-guard before touching
`jp->source`, and clear `jp->source` after delete to avoid a dangling
pointer.
- jsonpull/jsonpull.cpp: low-surrogate range check was comparing the
outer-loop byte `c` instead of the parsed code unit `ch`, breaking
surrogate-pair decoding for some \\uXXXX escapes. Pre-existing bug
preserved across the port.
- tile-join.cpp `handle_vector_layers`: require the field value to have
type JSON_STRING (and the key to be non-null) before calling
`string()`; the previous truthy `type` check would assert on a
non-string value.
Co-authored-by: Cursor <cursoragent@cursor.com>
Covers the `c` vs `ch` bug fixed in the previous commit: parsing "\uD83D\uE000" (a valid high surrogate followed by a non-surrogate BMP code point) used to mis-classify U+E000 as a low surrogate and combine the two units into U+1F400 (F0 9F 90 80). The fixed code flushes the stale high surrogate as standalone CESU-8 (ED A0 BD) and then encodes U+E000 normally as EE 80 80. Verified the test fails under the pre-fix logic. Co-authored-by: Cursor <cursoragent@cursor.com>
Profiling tl_2022_us_county.json (sample(1) on Apple Silicon) showed
~38% of parse time in allocator work and ~14% in std::string::push_back
during string-token construction. These changes target the low-hanging
fruit from that profile:
- Pre-reserve 2 slots in json_array and 4 slots in json_hash so
coordinate `[x, y]` pairs and typical GeoJSON property maps avoid
the 0 -> 1 -> 2 -> 4 vector-growth chain (and the shared_ptr copies
it incurs).
- Reuse a parser-wide std::string buffer for JSON_STRING tokens
instead of constructing a fresh local std::string per token. The
buffer is cleared (capacity preserved) at the start of each token
and copied into the final json_string, so once it has grown to the
longest string seen it stops reallocating entirely.
- std::move the freshly-created container shared_ptr into the parser
container stack in the `[` and `{` handlers, and move it out of the
frame on the matching `]` / `}`. Each move skips one atomic
inc/dec round-trip per container open and close.
On a tl_2022_us_county.json benchmark (4-iter user-time mean, Apple
Silicon, /usr/bin/time):
- main baseline: ~8.17s
- jsonpull-cpp before these changes: ~10.90s (+33%)
- jsonpull-cpp with these changes: ~9.33s (+14%)
So this commit recovers roughly half of the post-port regression.
The remaining gap is dominated by shared_ptr atomic refcount traffic
on the parse tree and per-node heap allocations, which would require
the larger unique_ptr/arena reworks to address.
Co-authored-by: Cursor <cursoragent@cursor.com>
In the C++ port, json_free was just `o.reset()`, which dropped the caller's reference but left the subtree alive: the parent's vector slot kept it allocated, and for line-delimited streams the parser's jp->root co-owned it until the next top-level value started parsing. That defeated the geojson-loop pattern of calling json_free on each feature after serializing it, which is supposed to release the feature so it doesn't sit in memory while subsequent ones are parsed. Restore the historical "remove this from the tree" semantics by splicing the node out of its parent (sharing splice_from_parent with json_disconnect) and clearing jp->root when the node is the parser's current top-level value, then dropping the caller's reference. Two unit tests pin this down: a pruning test parses "[[1, 2], [3, 4], [5, 6]]" element-wise and confirms that calling json_free on [3, 4] leaves the outer array with just [1, 2] and [5, 6]; a top-level test uses a weak_ptr observer to confirm that json_free on the parser's root really destroys the tree. Co-authored-by: Cursor <cursoragent@cursor.com>
Replaces the shared_ptr-based json_object_ptr with a unique_ptr that has a stateless custom deleter dispatching on json_object::type before calling the right subclass destructor. Eliminates per-node atomic reference-counting and the control-block allocation that shared_ptr required for every node in the tree. API now distinguishes owning and borrowing pointers explicitly: - json_read / json_read_separators / json_hash_get return raw json_object * (borrowed from the parser-owned tree). - json_read_tree / json_disconnect return json_object_ptr (caller takes ownership; back-pointers are cleared so the subtree can outlive the parser). - json_free / json_context / json_stringify take raw pointers. - The parser's container_stack holds raw pointers; jp->root keeps unique_ptr ownership of the most recent top-level value. Internally, take_from_owner moves the unique_ptr out of whichever parent vector / hash entry / parser root owned it, which both json_free and json_disconnect rely on. In the streaming parsers (parse_feature, parse_layers, the geojson-loop callback), we are careful to free `j` only after we have processed a complete Feature: json_read returns each token as the tree is being built up, and freeing an intermediate node would splice it out of the surrounding hash and corrupt the in-progress feature. Benchmark (tl_2022_us_county.json, -z0 --extend-zooms-if-still-dropping, median of 5 runs on macOS arm64): 8.5s, vs 10.6s with shared_ptr and 8.7s on the pre-refactor C baseline. Co-authored-by: Cursor <cursoragent@cursor.com>
Comment on lines
14
to
+16
| bool evaluate(std::unordered_map<std::string, mvt_value> const &feature, std::string const &layer, json_object *filter, std::set<std::string> &exclude_attributes, std::vector<std::string> const &unidecode_data); | ||
| json_object *parse_filter(const char *s); | ||
| json_object *read_filter(const char *fname); | ||
| json_object_ptr parse_filter(const char *s); | ||
| json_object_ptr read_filter(const char *fname); |
| run_prefilter_args rpa; // here so it stays in scope until joined | ||
| FILE *prefilter_read_fp = NULL; | ||
| json_pull *prefilter_jp = NULL; | ||
| json_pull_ptr prefilter_jp; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modernize jsonpull: C++ rewrite with
unique_ptrownershipSummary
Replaces the hand-rolled
malloc/realloc/freememory managementin
jsonpullwith a modern C++ object model: smart-pointer ownership,std::vectorfor arrays and hash entries,std::stringfor stringvalues, and a small inheritance hierarchy for the value-bearing types.
The motivation was to make
jsonpullless of a footgun (no morejson_freemistakes, no more readingo->value.string.stringfor anode that turned out to be
JSON_HASH) without giving up the abilityto inspect a partial tree, splice out subtrees, or hand a tree off to
outlive its parser. After several rounds of iteration, the final
shape is also a touch faster than the original C code on a real
workload.
What changed
Memory model
json_object_ptris now astd::unique_ptr<json_object, json_object_deleter>where
json_object_deleteris a stateless deleter that dispatches onthe discriminator and
static_casts to the right subclass beforedelete. No virtual destructor, no per-node vptr; subclass dtorsstill run, so the
std::vector/std::stringmembers get freed.std::vector<json_object_ptr>(arrays) or
std::vector<json_entry>(hashes, wherejson_entryisa
{key, value}aggregate that preserves insertion order).unique_ptrto the most recent top-level value injp->root;json_read_tree/json_disconnectare the supportedways to move ownership out of the parser.
std::string. Numbers store one of three representations(
double,unsigned long long,long long) in a discriminated union.Type-tagged subclasses
The original C
json_objectused aunionfor its value payload,which kept primitive nodes small. The first cut of the C++ port
couldn't reuse that union (the new members --
std::string,std::vector<json_object_ptr>, etc. -- are non-trivial and can't sitin an untagged union), so it widened every node to one struct
containing all possible fields. That ballooned the per-node cost to
168 bytes, even for
JSON_NULL/JSON_TRUE/JSON_FALSE.Splitting
json_objectinto a base class plusjson_number/json_string/json_array/json_hashsubclasses recovers mostof what the union was buying in C, plus a bit more via hoisting
parser-only
expectstate out of the data model and packingjson_number's representation into a discriminated union:json_object(TRUE/FALSE/NULL)json_numberjson_stringjson_arrayjson_hashCallers reach the typed payload through asserting accessors:
o->string(),o->number(),o->array(),o->entries(), etc.The accessors
assertthe type matches before downcasting, so theclass of bug where a
JSON_HASHis read as a string surfacesimmediately at debug time rather than as a silent garbage read.
Public API
Ownership is now explicit in the type signature:
json_read/json_read_separatorsjson_object *json_hash_getjson_object *json_read_treejson_object_ptrjson_disconnectjson_object_ptrjson_free(json_object *)voidjson_stringify(const json_object *)std::stringjson_readstill returns each parser token as the tree is being builtup (so callers can keep their existing "watch for a
JSON_HASHwhosetypefield is"Feature"" pattern), andjson_read_treenow clearsparent / parser back-pointers on the way out so detached trees can
outlive the
json_pullthey came from.Per-call-site changes
jsonpullis used in a lot of places, so the migration is broad(24 files, ~1800 added / ~560 deleted), but mechanical:
geojson-loop.cpp,read_json.cpp::parse_layers,plugin.cpp::parse_feature) iterate one feature at a time, hand itto
add_feature/ build aserial_feature, then calljson_free(j)to release it before the next iteration so already-serialized
features don't pile up in memory.
evaluator.cpp::read_filter/parse_filter,main.cpp) hold filters asjson_object_ptr; everywhere downstreamthat just needs to evaluate the filter takes a borrowed
json_object *.jsontool.cpp::join_csv) usejson_read_treeto take ownership and then mutate the resultingtree directly through the new subclass accessors.
pmtiles_file.cpp,geobuf.cpp,dirtiles.cpp,attribute.cpp) just read attribute hashes throughthe accessors; no ownership transfer needed.
Bug fixes flagged in code review
Bundled in as part of the port:
jsontool.cpp::out: route non-string values throughjson_stringifyinstead ofo->string(), which now asserts on anon-string type. Previously
--extractwould crash on numericattributes.
geojson.{hpp,cpp}::json_end_map: takejson_pull_ptr &so thecaller's
shared_ptris actually released, null-check beforetouching
jp->source, and clearjp->sourceafter delete.jsonpull.cppstring parser: the low-surrogate range check wascomparing the outer-loop byte
cinstead of the parsed code unitch, breaking decoding when a high surrogate was followed by anon-low-surrogate BMP code point. Covered by a new regression test
(
"\uD83D\uE000"used to combine into U+1F400; now correctly emitsthe stale high surrogate as standalone CESU-8 then U+E000 normally).
tile-join.cpp::handle_vector_layers: requireJSON_STRING(and anon-null key) before calling
string()so a non-string valuedoesn't trip an assert.
Performance
The first cut of the port (with
shared_ptrownership and per-tokenstd::stringconstruction) was a real regression. Subsequentcommits brought it back:
mainmain(pre-refactor C)shared_ptrshared_ptr+ cheap perf winsunique_ptr(this PR's tip)(
time ./tippecanoe -z0 -f -o /tmp/foo.mbtiles --extend-zooms-if-still-dropping tl_2022_us_county.json, Apple Silicon, 238 MB input.)The
shared_ptr→unique_ptrswitch alone was worth the largestchunk; the rest came from pre-reserving small
std::vectors, reusinga parser-wide
std::stringbuffer for string tokens to skipSSO-promotion / capacity-doubling on each token,
std::moveingcontainer pointers into and out of the parser's frame stack, and
packing
json_number's representation into a single 8-byte slot viathe discriminator.
Tests
unit.cpp:"\uD83D\uE000").json_freepruning: parsing"[[1, 2], [3, 4], [5, 6]]"token bytoken, calling
json_freeon the[3, 4]node, and asserting theouter array ends up with
[1, 2]and[5, 6].json_freeof the parser's current root really destroys the tree(verified by re-reading and checking
jp->rootis empty).make testpasses (10488 assertions across 9 test cases) plusthe entire
tests/integration suite, including the prefilter test(
tests/ne_110m_admin_0_countries/out/--coalesce_-z2_-Ccat.json.check)that turned out to be a useful canary for "we're freeing partial
parser state too early."
Commit-by-commit
shared_ptrandstd::vector/std::string.json_objectso primitives shrink from 168 to 24 bytes.expectstate out ofjson_object.json_number's three numeric slots into one union.jsonpullC++ port.json_freeactually free the subtree.jsonpulltounique_ptrownership.