kaizen: core_matcher: pool nfaBuffers and flattener in the test helper path#520
Open
sayrer wants to merge 3 commits into
Open
kaizen: core_matcher: pool nfaBuffers and flattener in the test helper path#520sayrer wants to merge 3 commits into
sayrer wants to merge 3 commits into
Conversation
matchesForJSONWithFlattener previously allocated a fresh *nfaBuffers
per call, and matchesForJSONEvent additionally allocated a fresh
*flattenJSON per call. These helpers are the path used by most tests
and benchmarks (the production *Quamina.MatchesForEvent already
reuses both).
Add two package-level sync.Pools and Get/Put on each call. On Put,
re-seat bufs.resultBuf with a fresh slice so the []X returned to the
caller can't be written through by the next pool user — the old
backing array stays exclusive to the caller.
Bench (Apple M1 Ultra, n=6):
NumberMatching-20 890.2n -> 498.3n -44.03%
2288 B -> 280 B -87.76%
10 allocs -> 3 allocs
geomean (10 benchmarks): -4.71% time, -18.39% B/op
Other benchmarks that route through *Quamina.MatchesForEvent rather
than the helpers are unchanged in behavior; a few show +1-3% noise
likely from code layout (they don't touch the pooled path).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sayrer
changed the title
timbray
approved these changes
Apr 17, 2026
timbray
reviewed
Apr 18, 2026
Owner
timbray
left a comment
timbray
left a comment
There was a problem hiding this comment.
Currently failing memory_cost_test, will sit on this till we get that stuff sorted out.
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
2 participants
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.
This one isn't a real win, but it's getting annoying to have the benchmarks with different behavior than the production path. When some flavor of more aggressive DFA optimization happens, this will be good.
matchesForJSONWithFlattener previously allocated a fresh *nfaBuffers per call, and matchesForJSONEvent additionally allocated a fresh *flattenJSON per call. These helpers are the path used by most tests and benchmarks (the production *Quamina.MatchesForEvent already reuses both).
Add two package-level sync.Pools and Get/Put on each call. On Put, re-seat bufs.resultBuf with a fresh slice so the []X returned to the caller can't be written through by the next pool user — the old backing array stays exclusive to the caller.
Bench (Apple M1 Ultra, n=6):
NumberMatching-20 890.2n -> 498.3n -44.03%
2288 B -> 280 B -87.76%
10 allocs -> 3 allocs
geomean (10 benchmarks): -4.71% time, -18.39% B/op
Other benchmarks that route through *Quamina.MatchesForEvent rather than the helpers are unchanged in behavior; a few show +1-3% noise likely from code layout (they don't touch the pooled path).
Tested #519 and #520 together under the race detector and it was clean, but it took 117 seconds.