Repository navigation
perf: reduce node snapshot and hash traversal overhead - #664
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
left a comment
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.
daveshanley
left a comment
There was a problem hiding this comment.
Re-checked d1a8313 and f8bfdca. Moving the identity check ahead of the hash is equivalent (both orders return keyOrigin), and the external branch only skips the hash when n.Node == value short-circuits anyway. Frozen reference copy is gone, cache tests no longer pin stale behaviour, hash tests are consolidated. Good to merge.
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.