Skip to content

perf: reduce node snapshot and hash traversal overhead - #664

Open
daveshanley wants to merge 5 commits into
mainfrom
codex/origin-hash
Open

daveshanley wants to merge 5 commits into
mainfrom
codex/origin-hash

Conversation

@daveshanley

@daveshanley daveshanley commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

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.

  • Add NodeMap.GetNodeLines() without changing GetNodes(). The returned slice is caller-owned and preserves line-zero, pending-write and concurrent-access behavior.
  • Skip hashing on identity matches for leaves and small composites; retain cache warming for nodes at or above the existing admission threshold. Preserve external-origin metadata and reference guards.
  • Hash leaves without traversal storage. Use local 16-pointer buffers for small composite traversals, with independent promotion to the existing pooled stack/map. Preserve exact bytes, preorder, cycles/shared nodes, cache admission and invalidation. Clear pooled pointers before return.

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:

  • Origin/leaf step: key/value sweeps use 22–42% less time in model mode and 30–44% less in graph mode, with 9–13% fewer allocations.
  • Small-subtree step, measured separately against the preceding step: another 9–22% less key/value lookup time in model mode and 9–23% less in graph mode. Allocation counts are effectively unchanged.
  • Small-composite hash microbenchmark: 56% less time; after a large traversal: 87% less. The 17/32-node chain boundary cases are 6–8% slower and cold-wide traversal is 4.6% slower; repeated large narrow traversal shows no regression in these samples.

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 main e0e2f846. 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 d1a8313b and f8bfdca1 replace 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 hashWalk extraction 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:

Spec Model before Model after Graph before Graph after
petstore.yaml 0.483 0.226 0.465 0.217
mistral.yaml 7.277 2.062 7.342 2.099
neon.yaml 8.004 2.318 7.736 2.335
ld.yaml 56.563 12.509 57.312 12.651
plaid.yml 90.354 19.263 88.345 19.491
stripe.yaml 272.977 132.361 333.311 145.389

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.

@codecov

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (c4c4be7) to head (f8bfdca).
⚠️ Report is 2 commits behind head on main.

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     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@daveshanley daveshanley left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread index/search_rolodex.go Outdated
Comment thread index/utility_methods.go
Comment thread index/origin_identity_reference_test.go Outdated
Comment thread index/origin_identity_test.go Outdated
Comment thread index/utility_methods_hash_fast_test.go Outdated
Comment thread index/utility_methods_hash_fast_test.go Outdated
Comment thread datamodel/low/node_lines_snapshot_test.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant