|
| 1 | +# ONEPASS_NFA findMatch()/findBoundsFrom() O(n²) fix — implementation plan |
| 2 | + |
| 3 | +**Branch/worktree:** `fix/onepass-findmatch-perf` at `.worktrees/onepass-findmatch-perf` |
| 4 | + |
| 5 | +## Problem |
| 6 | + |
| 7 | +`OnePassBytecodeGenerator.generateFindMatchFromMethod` (lines 1088-1145) and |
| 8 | +`generateFindBoundsFromMethod` locate the match start via `findFrom` (O(1) extra work — `findFrom` |
| 9 | +already delegates to the O(n) `matchFrom` single pass), then **discard the end position** and |
| 10 | +re-derive it by looping `matchEnd` from `matchStart` to `input.length()`, calling |
| 11 | +`matchesInRange(input, matchStart, matchEnd)` at every candidate length (each call allocates a |
| 12 | +substring and re-runs `matches()`). That's O(remaining input) work with allocation, on top of the |
| 13 | +O(1) end position `matchFrom` already computed and threw away. Confirmed via ad-hoc benchmark: |
| 14 | +`findMatch()` degrades to ~1.6-1.7x slower than JDK as haystack length grows (5k→20k chars), |
| 15 | +consistent with the redundant rescan. |
| 16 | + |
| 17 | +Boolean `find()`/`findFrom()` do NOT have this problem — they were already fixed in `6841723`. |
| 18 | + |
| 19 | +## Why the direct fix is safe here (and wouldn't be for other strategies) |
| 20 | + |
| 21 | +`matchFrom` returns as soon as it reaches the *first* accept state reached while scanning forward |
| 22 | +(`OnePassBytecodeGenerator.java:918-926`), before checking whether a longer match is possible. For |
| 23 | +a strategy handling nullable/optional-tail patterns (e.g. `ab?`), that would return the wrong |
| 24 | +(shortest) end position for a greedy match. |
| 25 | + |
| 26 | +**This does not apply to `ONEPASS_NFA`.** `PatternAnalyzer.isOnePassEligible()` |
| 27 | +(`PatternAnalyzer.java:90-124`) requires, for every NFA state: (1) no two character transitions |
| 28 | +have overlapping char sets, and (2) at most one epsilon transition. Quantifier branch points |
| 29 | +(`?`, `*`, `+`) always produce a state with two epsilon transitions (continue-loop vs. exit), so |
| 30 | +any pattern with a quantifier is disqualified from `ONEPASS_NFA` before this code path is ever |
| 31 | +reached — confirmed empirically: `(a)b?`, `([a-z]+)`, `(a+)`, `([a-z]*)x` all route to other |
| 32 | +strategies (`DFA_UNROLLED_WITH_GROUPS`, `SPECIALIZED_GREEDY_CHARCLASS`, `SPECIALIZED_CONCAT_GREEDY_GROUP`, |
| 33 | +`RECURSIVE_DESCENT`). Every existing `ONEPASS_NFA` test pattern (`([a-z])(bar)`, `(abc)`, `(a)$`, |
| 34 | +`^(a)`, `([a-z])$`) is a quantifier-free, fixed-length concatenation, which structurally has at |
| 35 | +most one possible accepting length per start position. So `matchFrom`'s returned position is |
| 36 | +*always* the unique correct end for any pattern that can actually reach this strategy — reusing it |
| 37 | +directly is correct, not just an optimization with a correctness trade-off. |
| 38 | + |
| 39 | +## Changes |
| 40 | + |
| 41 | +**File:** `reggie-codegen/src/main/java/com/datadoghq/reggie/codegen/codegen/OnePassBytecodeGenerator.java` |
| 42 | + |
| 43 | +1. `generateFindMatchFromMethod` (lines 1088-1145): replace the `matchEnd` trial loop with a |
| 44 | + direct call to `matchFrom(input, matchStart)` to get the end position in one O(match-length) |
| 45 | + pass, then call `matchInRange(input, matchStart, matchEnd)` for the `MatchResult` exactly as |
| 46 | + today (unchanged — this call already only fires once, at the resolved end, no allocation |
| 47 | + change there). Drop the now-unused `matchesInRange` trial loop entirely from this method. |
| 48 | +2. `generateFindBoundsFromMethod` (lines 1288+, same O(n²) shape per its own doc comment): same |
| 49 | + fix — call `matchFrom` for the end position directly instead of trial-looping. |
| 50 | +3. No changes to `matchFrom`, `findFrom`, `matches()`, or any other generated method — boolean |
| 51 | + `find()` perf (already fixed, 3.5x JDK) is untouched. |
| 52 | +4. No `StructuralHash`/`DFAState`/`PatternInfo` field changes — this only changes generated method |
| 53 | + bodies, not compiled pattern metadata, so no structural-hash update needed. |
| 54 | + |
| 55 | +## Tests |
| 56 | + |
| 57 | +**New file:** `reggie-runtime/src/test/java/com/datadoghq/reggie/runtime/OnePassFindMatchPerfCorrectnessTest.java` |
| 58 | + |
| 59 | +- Confirm routing to `ONEPASS_NFA` for the patterns under test via |
| 60 | + `StrategyCorrectnessMetaTest.routeOf(...)`. |
| 61 | +- Behavioral parity vs JDK for `findMatch()`/`findMatchFrom()`/`findBoundsFrom()` on: |
| 62 | + - `([a-z])(bar)` (existing meta-test representative) |
| 63 | + - `(abc)` at various positions within a longer haystack (start, middle, end, no match) |
| 64 | + - `(a)$`, `^(a)`, `([a-z])$`, `^([a-z])` (anchor cases, mirrors `OnePassNfaAnchorFindTest`) |
| 65 | + - A long-haystack case (≥10k chars) with the match near the end, to directly exercise the |
| 66 | + perf-motivated code path and catch any off-by-one in the new direct-end-position logic. |
| 67 | +- Existing `OnePassNfaAnchorFindTest` and `StrategyCorrectnessMetaTest` must continue to pass |
| 68 | + unmodified — they already cover `findMatch()` for `ONEPASS_NFA` and will catch a correctness |
| 69 | + regression from this change. |
| 70 | + |
| 71 | +## Verification steps |
| 72 | + |
| 73 | +1. `./gradlew :reggie-codegen:build :reggie-runtime:build` — compiles. |
| 74 | +2. `./gradlew :reggie-runtime:test --tests "OnePassFindMatchPerfCorrectnessTest"` — new tests pass. |
| 75 | +3. `./gradlew :reggie-runtime:test --tests "OnePassNfaAnchorFindTest" --tests "StrategyCorrectnessMetaTest" --tests "PikeVMRoutingTest"` — no regression. |
| 76 | +4. `./gradlew :reggie-integration-tests:test --tests "*AlgorithmicFuzzTest.divergenceGate_enforcedViaProperty" -Dreggie.fuzz.enforce=true -Dreggie.fuzz.maxFindings=28` — fuzz gate still passes at budget 28 (no new divergences). |
| 77 | +5. `./gradlew :reggie-benchmark:jmh -Pjmh.args="StrategyBenchmark.*Onepass.* -f 2 -wi 5 -i 5 -bm avgt -tu ns"` — confirm `reggieOnepassFind` unaffected (still ~3.5x JDK) and spot-check `findMatch` improvement if a benchmark method exists for it (add one if not, scoped to this task only if needed for evidence — do not expand into a general benchmark refactor). |
| 78 | +6. `./gradlew spotlessApply` before commit. |
| 79 | +7. Update `doc/agents-fallback-and-limitations.md`'s `ONEPASS_NFA` known-gap entry: either remove |
| 80 | + it (if fixed) or narrow it further based on actual measured results. |
| 81 | + |
| 82 | +## Out of scope |
| 83 | + |
| 84 | +- No changes to other strategies' find/match generators. |
| 85 | +- No new JMH benchmark infrastructure beyond what's needed to verify this specific fix. |
| 86 | +- No attempt to relax `isOnePassEligible()` to admit quantified patterns — that's a separate, |
| 87 | + much larger feature (would require a real greedy-longest single-pass algorithm, not this fix). |
0 commit comments