Repository navigation
perf: reduce node snapshot and hash traversal overhead - #664
Open
daveshanley wants to merge 5 commits into
Open
daveshanley wants to merge 5 commits into
daveshanley wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #664 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 309 309
Lines 39213 39240 +27
=========================================
+ Hits 39213 39240 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
daveshanley
commented
Oct 21, 2026
daveshanley
left a comment
Member
Author
There was a problem hiding this comment.
Logic is sound. Hash traversal matches the old byte stream (promotion order, visited handoff and pool clearing all check out), the identity short-circuit is equivalent to checkOrigin, vet + race + full index/datamodel suites pass locally, coverage on the changed funcs is 100%. Comments are mostly about test weight and maintainability.
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.
Large document walks and origin lookups repeatedly materialize node maps and acquire hashing scratch storage. This change adds an owned node-line snapshot and removes redundant hashing work while preserving existing lookup and hash behavior.
NodeMap.GetNodeLines()without changingGetNodes(). The returned slice is caller-owned and preserves line-zero, pending-write and concurrent-access behavior.Validation
Full libopenapi and Doctor suites, focused race checks, vet, frozen-reference contract tests, transition tests and independent performance review pass. Compiler escape analysis confirms the local buffers stay on the stack.
Isolated Vacuum and Bunkhouse consumer suites pass. Vacuum retains matching complete normalized diagnostics on all six specs (39,268 findings). Five Bunkhouse snapshots match; Stripe has one optional parent-path annotation that also varies on the unchanged baseline, detailed below. No field is excluded to hide that mismatch. Existing compound schema-message clause ordering was reproduced independently in baseline and candidate runs. Stripe's existing direct-build circular-reference error remains unchanged; linting succeeds.
Measurements and limits
Five alternating pairs on the six speed-test specs, three iterations per sample, Go 1.26.4 on Apple M4 Max:
These are incremental warm-library measurements, not additive percentages or end-to-end lint latency. Model-only walk timing is inconclusive; a Petstore counter probe confirms that workload makes zero HashNode calls. No peak/retained-memory reduction is claimed.
This branch is based on
c4c4be74; it merges cleanly with current maine0e2f846. Local benchmark baselines and test evidence use the recorded branch revisions; GitHub CI checks the PR integration. Full reports and raw samples are included in the companion Doctor PR.Companion Doctor PR, with full reports and raw benchmark samples: pb33f/doctor#122
Latest report: https://github.com/pb33f/doctor/blob/a25e66c/docs/performance/subtree-hash.md
Review follow-up
Review fixes in
d1a8313bandf8bfdca1replace the copied origin implementation with explicit expected origins, simplify cache tests, remove a no-op pool call and consolidate hash tests. The pool test is explicitly a smoke check, not proof of cleanup.A local
hashWalkextraction stayed off the heap but slowed small composites by 12–13% and large traversals by 7–8%; it was rejected and production traversal remains unchanged. Small origin identity matches now remove one allocation (208 to 192 B/op, three to two allocations) and use 45–62% less time in focused paired benchmarks.The full library/Doctor suites, race checks, vet and six-spec path/graph comparisons pass after review fixes. Vacuum retains all 39,268 findings. Bunkhouse suites pass and five full normalized snapshots match. Stripe has one existing nondeterministic optional parent-path annotation: the initial comparison differed, and all three unchanged-baseline repeats reproduce its omission. All 21,839 findings and every other field match; the strict comparison and original mismatch are retained without excluding that field.
Review-fix corpus measurements: five alternating pairs, three iterations per sample, same six speed-test specs. All 240 lookup samples preserve query/result/error counts and complete result signatures. Median milliseconds per full key/value sweep:
This is a 51.5–78.7% reduction in model-mode key/value sweep time and 53.3–77.9% in graph mode, incremental to b02d232. It is not end-to-end lint latency. Raw logs and records are retained in /tmp/pr664-review-evidence.