Repository navigation
perf: share per-NFA BitState setup bundles across matchers (-25% warm-cache compile+find) - #146
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| // null — always the case for BITSTATE originals, which take the usePikeVm engine route). Built | ||
| // eagerly here, before the entry is published into HYBRID_CACHE, so every matcher from the | ||
| // winning entry shares one bundle identity. PikeVM and generated NFA halves never touch it. | ||
| final BitStateMatcher.Bundle bitStateBundle; |
There was a problem hiding this comment.
Compiling many distinct BitState or BitState-backed hybrid patterns inserts entries into unbounded static caches. The new strong reference retains each full Bundle, including its growable LazyDFACache, until clearCache(), so high-cardinality workloads can exhaust the heap instead of allowing GC reclamation. Both entry types should soft-reference and rebuild these deterministic bundles.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
There was a problem hiding this comment.
Resolved by the rebase onto 2ad23b7 (#140) — the finding was accurate for the pre-rebase revisions (de921d1/cc8f5a0, eager final bundle fields), but current head (98b1a6e+60b663a) soft-holds both bundle types exactly as this review prescribes:
BitStateEntry.bundle→volatile SoftReference<BitStateMatcher.Bundle>with deterministic rebuild-on-miss (RuntimeCompiler.java:349, accessorbundle()at:363)HybridEntry.bitStateBundle→volatile SoftReference<BitStateMatcher.Bundle>, same rebuild contract (RuntimeCompiler.java:253, accessorbitStateBundle()at:304)
Live matchers pin their bundle via the BitStateMatcher.sourceBundle reachability anchor (generalizing #140's reject-bundle anchor), so eviction never affects in-flight matchers, and rebuilt bundles are equivalent because the build is a pure function of the NFA. #140's eviction tests (evictedPikeVmBundleIsRebuiltCorrectly, evictedBitStateBundleIsRebuiltCorrectly) pin the rebuild path, adapted to the full bundle.
What remains strongly held per entry is the entry itself (NFA + name map + SoftReference) in the pre-existing unbounded caches — unchanged by this PR and identical to the PikeVM/hybrid entries' contract since #139/#140.
Move all NFA-derived BitStateMatcher construction-time setup (flattened tables, greedy-loop shapes, single-first-ASCII prefilter, reject-DFA bundle with its LazyDFACache) into an immutable BitStateMatcher.Bundle. BitStateEntry and BitState-backed HybridEntry own one eager bundle published via putIfAbsent; matchers alias its read-only fields while all mutable state stays per-matcher. Warm-cache compile drops ~25% per-op (4461 -> 3333 ns/op best-of-7 JMH on a route-pinned GO-shaped hybrid fixture).
…nchmark Sharing identity (standalone + hybrid), mutable-state isolation, direct-ctor and counted-loop contracts, JDK-oracle API parity, clearCache lifecycle, entry publication and LazyDFACache population under contention (latch-released workers, capped at 8, fixed 40k total ops). The benchmark measures one warm-cache compile()+find() and fails loudly if routing drifts away from the BitState-backed hybrid.
cc8f5a0 to
3e7833d
Compare
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 3 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 3e7833d | Docs | View more details | Give us feedback! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
=======================================
Coverage 84.9% 84.9%
Complexity 1 1
=======================================
Files 163 163
Lines 49002 49021 +19
Branches 7009 7009
=======================================
+ Hits 41613 41634 +21
+ Misses 5379 5378 -1
+ Partials 2010 2009 -1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
2 participants
What does this PR do?
Shares all NFA-derived
BitStateMatcherconstruction-time setup across matchers built from one cached NFA. The flattened per-state tables, greedy-loop shapes, single-first-ASCII prefilter, and the reject-DFA bundle (with itsLazyDFACache) move into an immutableBitStateMatcher.Bundle;RuntimeCompiler.BitStateEntryand BitState-backedHybridEntryown one eager bundle published viaputIfAbsent, and every matcher aliases its read-only fields. All mutable matcher state (capture buffers, job stacks, visited bitmap, counters, Laurikari instance, lazy PikeVM fallback) stays per-matcher;compile()still returns a fresh matcher per call.Warm-cache per-operation
compile()+find()drops from 4461.5 to 3333.3 ns/op (best-of-7 JMH, −25.3%), closing most of the remaining construction-cost gap on the backend's per-op matcher creation path.Motivation
The profiling-backend wave-3 migration constructs generated matchers per operation through the runtime cache; per-construction table rebuilding was the measured dominant cost on the BitState path (cf. PR #139, which did the same for the PikeVM
DfaBundle). Draft source:.issue-drafts/1-bitstate-table-sharing.md.Related Issue(s)
Implements
.issue-drafts/1-bitstate-table-sharing.md(draft measured headroom confirmed).Change Type
Checklist
./gradlew build)Performance Impact
Best-of-7 JMH (route-guarded fixture
^((?:x|y)+?)\.([^/]+), verified BitState-backed hybrid; 5×1s warmup, 5×1s measurement, 1 fork, sequential runs): baseline 4461.5 ns/op → 3333.3 ns/op, −1128 ns/op (−25.29%). All 14 raw JSON runs and the comparison retained in the session artifacts. Cold compilation, analysis, and budgets are untouched (BitState compile cost is analysis-side —kb/hyp-bitstate-blowup-root).Additional Notes
SharedDfaBundleTestpins: sharing identity (standalone + hybrid), full mutable-state isolation, direct-ctor/counted-loop contracts, JDK-oracle API parity,clearCache()lifecycle, and entry publication + concurrentLazyDFACachepopulation under contention. An independent concurrency-focused review verified safe publication and read-only discipline of every aliased table. Out of scope (filed separately): generated-DFA-half region/^anchoring divergence on bounded matching with non-zero region start.Rebase note (post #140)
Rebased onto 2ad23b7 (#140, soft-held shared NFA bundles) and reconciled: the full
BitStateMatcher.Bundleis now soft-held per cache entry (BitStateEntry.bundle()/HybridEntry.bitStateBundle()) with #140's deterministic rebuild-on-miss; live matchers pin their bundle via aBitStateMatcher.sourceBundlereachability anchor (generalizing #140's reject-bundle anchor). The original eager-final publication guarantee is superseded by #140's equivalent-bundle-on-rebuild contract; #140's eviction tests were adapted to the full bundle and all tests pass (3278 green, 0 failures).Measurement after rebase (same benchmark): the machine is currently too noisy for a precise figure (±10% run spread). Interleaved same-epoch A/B vs main@2ad23b7 (4 runs/side): median −13.1%, branch won the last 3 paired comparisons; best-of comparison vs the original 967e525 baseline remains −21.8%. Direction consistently favorable; will re-measure on a quiet machine before removing the Draft label.
Linux A/B (workspace-jb, 16-core x86_64, Zulu 26.0.2.1, quiet box)
Strictly interleaved runs, 7 per side, same machine/JDK (
main@2ad23b7vs this branch, route-guarded benchmark):Direction consistent with the macOS measurements; run spread on this box is ~3%. Raw JSONs for all 14 runs retained (linux-workspace-jb/).