perf(regex): decide a per-call search in one engine entry - #10580
proggeramlug wants to merge 3 commits into
Conversation
find_near and find_near_lent each built a Search and then advanced it: two view acquisitions per JS regex call, and a Search moved between them, for a search whose first quantum decides it. Search::run acquires the views once and runs that quantum in the same borrow, so a decided search never builds a Search at all. Run::Paused falls into the existing advance loop unchanged, and a pause from a Frames/Undo shortage re-raises on the next advance (perex keeps it in state.blocked) straight into the scratch-growth branch, so both paths behave exactly as Search::new followed by advance did. Instructions per call, control subtracted, one Perry commit and one perex commit differing only by this patch, release build, min of 5 interleaved rounds on a 16-core Linux host: .test() anchored 4,382.2 -> 4,063.7 -7.3% .test() inline literal 5,698.2 -> 5,379.7 -5.6% exec, two groups 7,566.9 -> 7,237.9 -4.3% .test() unanchored miss 2,438.7 -> 2,133.7 -12.5% .test() unanchored hit 5,813.1 -> 5,481.1 -5.7% 200,000-char subject 1,439,713 -> 1,439,003 -0.0% 305 to 332 instructions whatever the pattern, which is the fixed cost of the call; it vanishes against a subject long enough for matching to dominate. Every probe returns an identical answer on both arms and on Node 26.5.1. Wall-clock figures are deliberately omitted: the measuring host carried other sessions' builds throughout, and round-to-round spread reached 68-244% against effects of 0.5-25%, enough to invert the sign of a known-good result. Only instruction counts are quoted. perex 0.1.9 supplies Search::run and Run. Its src/ is byte-identical to the commit the figures above were measured against, and re-measuring the released crate reproduced every row to 0.1 instructions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe PR updates the ChangesRegex search update
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant RegexRuntime
participant SearchRun as Search.run
participant Run
RegexRuntime->>SearchRun: start search with resources, position, budget, and quantum
SearchRun-->>Run: return Finished or Paused
Run-->>RegexRuntime: provide remaining work, match, or paused state
RegexRuntime->>RegexRuntime: continue Paused results through advance
Merge Risk: ⚪ Minimal · up to The regex execution update preserves current paused-search behavior and has no concrete merge-blocking risk at the current head. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Update: the engine-side review the description asked for has happened, from the perex side. The scratch-growth interaction I flagged as having no obvious owner was checked and holds: That review surfaced one real behavioural difference, which I've now documented in the code (third commit) rather than left implicit: On an error that is not a capacity request — This is not observable on any path in this PR. I checked rather than assuming: every I deliberately did not invent a workaround. The only contained one is to guess a charge on the error arm (e.g. deduct the whole quantum), which trades a silent under-count for a silent over-count and would break exactness against the two-step path. perex would rather give the failure arm a remaining-work figure and hand the buffers back, which is additive and changes nothing here; I'd rather wait for that than encode a guess. What the comment buys in the meantime: #10164 and #10165 are both about regex work running away, and a plausible fix for either is to reintroduce a finite execution budget. If that happens, this stops being free and starts silently under-charging failed calls. The comment names that at both call sites so it is tripped over rather than rediscovered. One smaller note, also recorded so nobody rediscovers it as a mystery: poll cadence shifts by one quantum on the paused path. Previously every |
|
Landed via merge train #10631 (v0.5.1595). All source commits preserve authorship; merged main matches the validated train exactly. |
Adopts perex's one-shot search entry on Perry's two host search paths. Continues the per-call work on #10166.
What it does
find_nearandfind_near_lenteach built aSearchand then advanced it. That is two view acquisitions per JS regex call, plus aSearchmoved between them — for a search whose first quantum decides it in nearly every case.Search::runacquires the views once and runs that quantum in the same borrow, so a decided search never builds aSearchat all.Run::Pausedfalls into the existing advance loop unchanged. A pause caused by aFrames/Undoshortage is carried in perex'sstate.blockedand re-raised by the nextadvance, so it lands in the existing scratch-growth branch exactly as before — both paths behave asSearch::newfollowed byadvancedid.Measurement
Instructions per call, control subtracted (an identical binary building the same 1M-string array and running the same loop with a trivial predicate in place of the regex), so these are the regex call alone. One Perry commit and one perex commit, differing only by this patch — the figures isolate
Search::runand contain nothing else from the 0.1.9 release. Release build, min of 5 interleaved rounds, 16-core Linux host..test()anchored, 50% hit.test()inline literalexec, two capture groups.test()unanchored miss.test()unanchored hit305 to 332 instructions whatever the pattern — the fixed cost of the call. The last row is the control on the claim: a fixed per-call saving correctly disappears against a subject long enough for matching to dominate.
All six probes return identical answers on both arms and on Node 26.5.1.
Wall-clock figures are deliberately omitted. The measuring host carried other sessions' builds throughout; round-to-round spread reached 68–244% against effects of 0.5–25%, enough to invert the sign of a result the instruction counts show clearly. Quoting a minimum from that would have been misleading, so only instruction counts appear here.
Dependency
perex = "0.1.7"→"0.1.9", forSearch::runandRun. 0.1.9'ssrc/is byte-identical to the commit the figures were measured against, and re-measuring against the released crate reproduced every row to 0.1 instructions per call.Resolving 0.1.9 needed
CARGO_RESOLVER_INCOMPATIBLE_PUBLISH_AGE=allowonce, because.cargo/config.tomlsoaks releases for 7 days and 0.1.9 was hours old — authorised by Ralph for this version. The lock entry is committed, so no override is needed to build this branch:validate-017.sh's first gate, the--lockedbuild without the override, passes.Validation (local; runners are unreliable)
perry-runtimelib suite: 3984 passed, 0 failed, 4 ignored--lockedbuild without the override: OKcargo fmt --check: OK · regex-off-D warnings: OK · product-D warnings: OKc8cf45056with no changes applied. Not introduced here.Considered and not done
Search::run_without_capturesfor.test(). perex measured the capture round a boolean search skips at 88 instructions per call — a 2.2% ceiling against a 4,064-instruction call — while Perry'sfind_nearbuilds its match span fromcapture(0)in both capture modes, so serving it means either a separate boolean host entry or makingMatch.fulloptional across ~12 readers. Priced and rejected; Perry's per-call cost is host-boundary bound, not engine bound.QUANTUM = usize::MAX, which would let perex choose its pause-check-free trial compilation. Worth a further −1.2% to −2.0% on short calls and −5.5% on a 200k-character subject, but it removes every pause point inside a single search, so it is a Perry policy change rather than an adoption. Measured, not proposed here.Review note
The one part of this with any subtlety is the interaction with the lent path's scratch-growth branch. I verified it by reading perex's
advance(state.blockedre-raises before anything else) and the 200k-character probe drives the paused path thousands of times per run with correct results — but the session that wrotefind_near_lenthas ended, so that review has no obvious owner. Worth a second pair of eyes there specifically.Summary by CodeRabbit