Fix/empty hash and perf - #25
Merged
Merged
Conversation
… deep cloning
- diff_and_common_multiple: a value of {} at any depth (e.g. ArgoCD's
syncPolicy: {automated: {}}) produced neither base keys nor diff entries
and was silently dropped. Treat the empty hash as an atomic value: it
becomes the base when it meets the quorum, otherwise each empty-hash
file keeps {} in its diff.
- diff_and_common_multiple: move the accumulated base subtree into the
parent instead of deep-cloning it at every recursion level.
- merge_yaml/sort_yaml: clone the document once and mutate in place
instead of re-cloning subtrees per recursion level.
Allocation is now depth-independent (diff 1.86x->1.04x of input, merge
5.0x->1.13x, sort 6.2x->2.08x on 87k-node docs); merge -76% and sort -70%
wall time, multi-file diff -20..-36%.
- benches/core_benches.rs: cargo bench baseline for compute_diff, diff_and_common_multiple, merge_yaml, sort_yaml at two document sizes. - benches/alloc_benches.rs: counting global allocator reporting bytes, allocation count, peak growth, and allocated-bytes-to-input-size ratio per operation; a ratio growing with tree depth indicates redundant subtree cloning. - benches/common: synthetic GitOps-values-shaped document generator.
The old os x arch matrix built natively on every runner, so the arch dimension only duplicated jobs. Use ubuntu-latest (amd64) and ubuntu-24.04-arm (arm64) instead.
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.
Summary
Fixes a correctness bug where empty-hash values silently vanish during base extraction, eliminates depth-scaled deep cloning in the three core tree operations, and adds a benchmark suite to lock in the performance baseline.
Correctness: empty-hash values were lost
A key whose value is
{}at any depth — e.g. ArgoCD's ubiquitoussyncPolicy: {automated: {}}— ended up in neither the base nor any diff. In the hash branch ofdiff_and_common_multiple, an empty hash contributes no keys, so it produced no base entry and no diff entry and simply disappeared, breaking themerge(base, diff) == originalround-trip.The fix treats the empty hash as an atomic value:
{}keeps it in its diff.New tests cover the shared case (all files agree → lands in base), the below-quorum case (outlier file keeps
{}in its diff), and the quorum-met-with-outlier case.Performance: allocation no longer scales with tree depth
Three functions deep-cloned accumulated subtrees once per recursion level, making allocation O(depth × size) instead of O(size):
diff_and_common_multiplecloned the accumulated base subtree at every level (sub_base_val.clone().into_owned()— the clone only existed becausebase_includes_keyreadsub_baseafterwards; reordering removes it)merge_yamlcloned every hash level and re-cloned subtrees along override paths → now clones the base once and merges in placesort_yamlcloned each node and re-cloned children per level, and re-parsed thepreOrderconfig at every hash node → now clones once, sorts in place, parses config oncePublic APIs and behavior are unchanged.
Benchmarks
benches/core_benches.rs(criterion, wall time) andbenches/alloc_benches.rs(counting global allocator: bytes, allocation count, peak, allocated-bytes/input-size ratio), driven by a synthetic GitOps-values-shaped document generator.Measured on 87k-node documents (8.5 MiB each):
diff_and_common_multiple(4 docs)diff_and_common_multiple(10 docs)merge_yamlsort_yamlcompute_diff(untouched)The alloc ratios are now depth-independent.
sort_yaml's remaining ~2x is the hash-container rebuild inherent to reordering aLinkedHashMap(constant, not depth-scaled).Testing
cargo test— 26/26 pass, including 3 new empty-hash testsbase.yamlwith each generated diff structurally reproduces every original file, including the previously-lostsyncPolicyblockscargo benchbaselines stored for future regression comparison