perf: compile user regex patterns once per planned expression - #5612
perf: compile user regex patterns once per planned expression#5612dwsmith1983 wants to merge 27 commits into
Conversation
regexp_extract, regexp_extract_all, and split compiled the user pattern with Regex::new inside the per-batch evaluation path, so every 8192-row batch paid a full regex compile. The pattern cannot be hoisted to construction time because these are scalar functions created by name, with the pattern arriving per invocation as a scalar argument. Each planned expression now owns a one-slot pattern cache that compiles only when the pattern string changes, the same cost model rlike already has. Error messages and the phase at which an invalid pattern fails are unchanged. regexp_extract drops from 862us to 705us per 8192-row batch on the criterion bench, and a small-batch run (512 rows, 5000 batches) is 2.1x faster. split is unchanged on literal delimiters, which never compile a regex.
9428e6a to
b4d9152
Compare
sunchao
left a comment
There was a problem hiding this comment.
Reviewed b4d9152367c8a0233beaa8b9817a69c0969e0c11 against 8729f6e6adf7091e18a48670e790d4ba8fd41e51. No verified P1/P2 findings.
A focused check of the unchanged cache source passed six tests, including concurrent cold access, different-pattern replacement and invalid-pattern recovery. This was a cache-only check, not the full Comet/Spark suite. Current-head workflows report action_required. The earlier HEAD's green checks are not current-head validation.
Could you add a matched BASE/HEAD multithreaded benchmark with shared-UDF and per-worker controls, using 1/2/4/8 workers and 512/8192-row batches? Please cover cold and warm caches, alternating patterns, and a regex delimiter for split, and report throughput, batch latency and allocations while checking equal results and confirming the native path. Regex::clone() shares the compiled program but creates a fresh search-cache pool, so this would measure both contention and the per-batch clone cost.
|
@sunchao Warm regime, per worker instances (matches real plans, where the pattern is a literal and each task gets its own expression instance):
Main anti-scales on small batches: 8 threads run slower than 1 because every thread recompiles the pattern per batch and the compiles hammer the allocator. This PR scales near linearly. Allocations per 512 row batch for regexp_extract drop from 988 to 82 (the compile alone is roughly 900 allocations and 0.7 MB). Per batch latency follows the same shape, for example 699us mean / 1271us p99 down to 62us / 106us in the 8 worker 512 row cell. Worst case for the one slot cache, a pattern that alternates on every single invoke: within 2 percent of main across all three functions and both batch sizes, since the miss path pays the same compile main always pays plus an uncontended mutex. Cold first invoke on a fresh instance is also unchanged (for example 415us on main vs 403us here for regexp_extract on 8192 rows). One honest caveat: an artificial control where a single UDF instance is shared across 8 threads simultaneously regresses regexp_extract_all on 8192 row batches by 6 to 29 percent. The threads contend on the shared compiled Regex's internal scratch pool in that setup, while main sidesteps it by compiling privately per batch, which is the same behavior causing the anti-scaling above. That configuration does not occur in Comet since each task deserializes its own plan and gets its own expression instance, and regexp_extract and split win in shared mode anyway. |
|
@dwsmith1983 Thanks for covering the requested matrix. Could you attach the runnable harness/commands, exact baseline and PR commit SHAs, dependency/build settings, and per-cell results for both replicates, including the regressing shared-instance cases? Per-task plan ownership does not rule out sharing within a task. Source inspection shows that Comet passes sort-key expressions directly to Could you also revisit the scratch-pool attribution? |
With the pattern cache handing every invocation a clone of one compiled regex, captures_iter became a bottleneck under concurrent evaluation of the same expression (a sort key evaluated by parallel sort streams): each per-match Captures clones the program's shared group-info Arc, and that refcount turns into a contended cache line. Drive iteration with find_iter, which yields plain spans with identical semantics, and resolve groups through one reused CaptureLocations per batch, matching what regexp_extract already does. This removes the contention and the per-match allocations.
|
Thanks for pushing on all three points. You were right on both technical claims, so taking them in order. Harness and raw data: https://gist.github.com/dwsmith1983/e46e22c1c594b4f5120515c773f2b3ef has the full harness source, exact build and run commands, both commit SHAs, toolchain and dependency versions, and per-cell CSVs for both replicates including the regressing shared-instance cells. Sort path: your reading of ExternalSorter checks out and the repro confirms it. With the exact plan shape Comet produces (SortExec, no fetch, single partition, 128 x 8192-row batches so the reservation is well past sort_in_place_threshold_bytes), a tracking shim around one UDF instance measured max 9 concurrent in-flight evaluations at 8 runtime workers, and even 2 at 1 worker since the merge evaluates concurrently with a spawned sort task. On that path regexp_extract_all as the sort key was 1.45x slower than base at 1 worker and 2.2x at 8. regexp_extract as the key was parity to slightly faster. Attribution: you were right that my scratch-pool explanation was wrong. Clone creates a fresh private pool (meta/regex.rs 1916-1926), so scratch state is never shared. The real mechanism, isolated in a micro benchmark in the gist, is per-row refcount traffic: captures_iter creates a Captures per row via create_captures, which is Captures::all(self.group_info().clone()), an Arc clone against the program-owned GroupInfo, plus one more Captures clone per match. With every thread holding clones of one compiled program, that single refcount cache line bounces across cores and caps throughput regardless of thread count. A variant with one clone per thread, no lock and no per-invoke clone still collapses identically, which rules out the mutex and the clone itself. regexp_extract is immune because it reuses one CaptureLocations across rows, and split never creates a Captures. That pointed at the fix, now pushed: regexp_extract_all drives iteration with find_iter (identical span semantics, verified against the crate's shared iterator code and pinned with empty-match and multibyte edge tests) and resolves groups through captures_read_at into one CaptureLocations reused per batch, same as regexp_extract. Rerun results: the sort scenario goes from 2.2x slower to 8 percent faster than base at 8 workers, shared-instance and per-instance modes are now identical, and removing the per-match allocations lets the function scale near linearly to 8 workers (7.1 to 72 Mrows/s at 8 workers, where base and the previous head were both stuck near 7). Outputs stay byte identical across base and both head builds in every cell. Fix verification tables and CSVs are in the gist as well. |
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the capture-location update in 84f9ee006fc94fb592b61439d9d520d484af616d and the pinned benchmark follow-up. No new P1/P2 findings.
A focused regex-only comparison passed for 86 patterns and 1,512 strings, including empty matches, optional groups, anchors, word boundaries, and UTF-8 offsets. The benchmarked fix has the same relevant source as this HEAD, and the supplied sort results show recovery of the reported regression. Those timings are author-run evidence, not my measurements. I did not run the full Comet/Spark suite; the final CI snapshot had 37 passing checks, 28 queued/running, and 6 skipped.
sunchao
left a comment
There was a problem hiding this comment.
Rechecked d0fea134. The PR-only patch is byte-for-byte identical to the previous reviewed pair, and the head-to-head change exactly matches the base update. I checked the affected integration paths and found no new P1/P2. The existing approval stands.
The relevant regex code and dependencies are unchanged, so prior component evidence was reused without rerunning tests. This head has no check runs and three workflows awaiting approval.
|
The two failing legs here are infrastructure, not the change: spark-sql-sql_hive-3 died fetching compiler-bridge from Maven Central (java.net.SocketException, Network is unreachable) while compiling Spark itself, and the Iceberg 1.11 leg could not fetch the shadow plugin from plugins.gradle.org, same flake that hit #5568 last week. Neither reached any Comet code. Could someone with access rerun those two jobs when convenient? |
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed a7d2b8af394262030c35899b8d42e7ef8ac4206c against ef62b463. The regex implementation and dependencies are unchanged. I checked the incoming remote dictionary-decoding path at the regex input boundary and found no new P1/P2. The existing approval stands.
This was a source/integration review. The earlier regex-only probe remains applicable by source/dependency identity, but I did not rerun it or execute a Spark/Comet query or benchmark. The three current-head workflows still require authorization, with no executed check results reported.
|
Found one P2 performance regression on
My independent reproduction measured:
Three runs agreed. These measurements include baseline compilation, PR cache lookup/cloning, and Arrow output construction/destruction. They use the exact source and pinned dependencies, without allocation-counting instrumentation. They measure native function calls, not whole Spark queries. The cache lifetime itself is correct: production reuses the captured cache; Validation passed:
I would retain the cache optimization and revise the capture iteration before merging. [P2] Avoid searching every regex match twice The find_iter/captures_read_at combination regresses complete native regexp_extract_all calls on matched inputs. Three optimized exact-source runs, including an independent repeat, measured about 37–39% more time for 8,192 rows of '123-456-789-123' with pattern '(\d+)' and group 1, and 44–45% more time for 512 rows of 8KB strings matched by '(a+)'. Measurements include baseline compilation, the new cache lookup/clone, and Arrow output construction/destruction. Please preserve the cache benefit while avoiding this repeated matching work, and cover these cases in benchmarks. |
Drive the match walk with captures_read_at into one reused CaptureLocations instead of find_iter followed by a second capture search per match. The walk follows the regex crate's iterator rule for empty matches, so results are unchanged, and an equivalence test checks it against captures_iter over empty-match patterns, multibyte haystacks, and out-of-range groups. The benchmark gains the short-row and 8 KB-row cases that exposed the double search.
|
Thanks, that double search was real and it reproduced here at +24 percent and +43 percent on your two cases. Fixed in ea8c0e3: the loop now drives Your two cases are now in
Full table is in the commit's bench file output; every case is below base after the fix. |
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked 7f5bbc752b0afa5b7b4f03b87b02a12b11bccde2 against authoritative base fefee03d94045ecd0ac5d3a1edb98a555f5ff21d, including the changes since the previous review. The new regexp_extract_all loop preserves the regex iterator's match order: it advances to the overall match end, retries one byte later when an empty match repeats that end, and stops before calling captures_read_at beyond the input. It searches the original haystack, preserving anchor and boundary context. I checked this against the current regex-automata 0.4.18 source and found no new correctness issue.
The maintained Spark 3.5/4.0 implementations remain the semantic reference: null propagation and unmatched optional groups were checked, along with Spark's match-dependent group-index validation and 4.0 collation flags. This patch preserves the native path's existing error timing and regex differences; the default path still uses Spark's codegen dispatcher. Maintained 3.4/4.1 source coverage is unavailable.
CI's actual checkout b094602e9 has the assigned base/head parents and a tree identical to head. Lint, all-target Clippy, benchmark compilation and the native test job passed. The native run finished with 1,261 passed and five skipped. Its log includes the new 576-comparison iterator test, all three cross-batch cache tests, invalid-pattern recovery and null/type cases. At September 9, 03:40 UTC, 18 checks had succeeded, six were skipped and five were still running, with no failed check. Spark runtime tests are still pending. No local native or performance run was performed.
Performance
The reported P2 double-search regression is addressed: both scalar and array paths advance directly from the capture result, keeping one capture-location buffer per invocation/batch. They do not restore the per-match capture allocation that caused the earlier shared-expression regression. The two adverse inputs are now committed benchmark cases.
The author's response reports 1.93→1.44 ms for the short digit rows and 59.2→48.2 ms for the 8 KB rows. The fixed source and benchmark are byte-identical to that fix commit. These are author measurements against 55ae4f20, with DataFusion 54.1.0/Arrow 58.4.0/regex-automata 0.4.16; current head uses 55.0.0/59.3.0/0.4.18. The older gist measures the preceding two-search fix. I have not reproduced current-head timings or concurrency results, and do not treat those historical numbers as current-stack qualification. No remaining P1/P2 found.
Design
The cache remains owned by the planned UDF closure and survives batch evaluation and expression cloning. A pattern change recompiles under the mutex, while matching uses an owned regex clone after the lock is released. An invalid replacement leaves the previous valid entry intact without serving it for the invalid pattern. Capture buffers stay invocation-local, which matters because DataFusion's sort path can still share an expression across concurrent evaluations.
Abstraction & complexity
One small match-walking helper now serves scalar and array extraction, keeping their cursor rules aligned without another cache layer. The bounded one-entry cache remains straightforward. Regex cloning still creates a private search-cache pool and has a cost; the change removes compilation and per-match capture allocation rather than making invocation allocation-free. I found no separate complexity issue in this update.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked 9355197986704d166571926cf6fd4ac9885819e2 against 5627ab8c01473b186db2fbe73c91c599b1387f9b, using the prior source review at 7f5bbc75. The ten-file authored patch is byte-identical to that reviewed version. The complete head-to-head increment equals the base update, with no regex-source changes or merge-specific edits. No new or remaining P1/P2 found. The existing approval already applies to this head and stands.
I rechecked the incoming JNI exception, cached-string statistics and Parquet conversion interactions. The new typed exception walker does not intercept native regex execution errors or the dispatcher's original JVM throwable. The string input and regex planning paths are preserved. Against maintained Spark 3.5/4.0, the prior assessment is unchanged: default Spark codegen dispatch, optional-group/null behavior and the native path's existing error-timing, regex-engine and dynamic-column-pattern limitations remain as reviewed. This merge adds no new compatibility coverage. Maintained 3.4/4.1 source branches remain unavailable.
At 2026-09-09T14:49:10.801863+00:00, this head has one successful labeling check. CI, CodeQL and the Delta gate require authorization and have zero jobs. I reverified the earlier native CI checkout's parents/tree and the identical regex, planner, serde and dependency sources. Its 1,261 passed tests, including the 576-case iterator comparison and cross-batch cache tests, remain component evidence for those unchanged paths. They are not current-merge execution. No local product test or benchmark was run in this follow-up.
Performance
The previously reported double-search P2 remains fixed. Both scalar and array regexp_extract_all still advance from the capture result and reuse an invocation-local capture buffer. The merge reintroduces neither the redundant match search nor the per-match shared capture allocations. All three benchmark files and the dependency lock are unchanged from the prior review.
The earlier author timings remain historical evidence on the older dependency stack. There are no new benchmark results in the discussion, and I do not infer a current-stack speedup or concurrency result from those numbers. The update adds no cost to the reviewed regex hot paths.
Design
The base update preserves planned-expression ownership of the cache. Pattern changes are checked on every scalar invocation, invalid replacements return an error without serving the previous entry, and returned regex clones remain valid across later replacements. Matching happens after the cache lock is released. DataFusion's unchanged sort path can still share the expression across concurrent evaluations, so the existing private capture buffers remain necessary. No new design issue emerged from this merge.
Abstraction & complexity
There is no new regex abstraction or indirection in this update. The existing one-entry cache and shared match-walking helper remain unchanged, as do their production call sites. I found no update-specific complexity issue requiring a change.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked f4d4c2c083f90874481212206f406b3819c97a29 against 424c31aa79d13fddf743ffa29bae3c6f146e6c5e, including the complete increment from 93551979. The ten-file regex contribution is byte-identical to the previous reviewed pair. All six incoming files match the new base, and the entire head-to-head patch equals that base update. No new or remaining P1/P2 found. The current-head approval stands.
The incoming change removes the dedicated FIRST/LAST partial-merge refusal and updates the aggregate tests. I checked its interaction with expression conversion: grouping and partial-input expressions still use the same serializers, and partial-merge/final expressions still bind through the native buffer path. It does not change regex registration, cache ownership, argument handling or capture iteration. The new aggregate fixture uses values constant within each group, consistent with maintained Spark 4.0's order-dependent FIRST/LAST semantics. That fixture does not provide new regex validation.
Against maintained Spark 3.5/4.0, the regex assessment is unchanged: default execution still uses Spark's codegen dispatcher, and null/optional-group handling and match-dependent group-index validation remain the reference behavior. The opt-in native path retains its previously reviewed Java/Rust regex, eager-error and dynamic-pattern limitations. This base merge adds no compatibility claim. Maintained Spark 3.4/4.1 sources remain unavailable.
Validation and limits
At 2026-09-09T18:37:47Z, the only current-head check is successful labeling. CI, CodeQL and the Delta gate await authorization and have zero jobs. The current merge d8516c96 has the assigned base/head parents and a tree identical to head, but no product tests have executed on it.
I freshly re-read the earlier native CI job, its actual checkout parents/tree, and sixteen unchanged regex/planner/serde/dependency-source files. Its 1,261 passed tests and five skipped, including the 576-case iterator comparison and cross-batch cache tests, remain evidence for those unchanged components. They are not execution of this aggregate-planning update. All existing discussion bodies were read. The only review added since the previous packet is my published follow-up. No local product test or benchmark was run in this round.
Performance
The earlier double-search P2 remains fixed in both scalar and array regexp_extract_all: the helper advances from captures_read_at and reuses an invocation-local capture buffer. This update changes none of that hot path or the three benchmark files. It introduces no additional regex lookup, lock, clone or per-match allocation.
The author's earlier timings remain measurements on the older dependency stack. Current source still matches the recorded fix, but I have not measured current-stack throughput or concurrent execution. The merge supplies no new benchmark result.
Design
The aggregate-planning update preserves the existing UDF lifetime and cloning path. A changed scalar pattern still replaces the one cache entry under the mutex, an invalid replacement does not serve the previous pattern, and matching runs on an owned clone after releasing the lock. The unchanged DataFusion sort path can share expressions across evaluations, so the invocation-local capture state remains necessary. No new interaction requires a design change.
Abstraction & complexity
No regex abstraction or indirection was added in this follow-up. The shared match-walking helper and single-entry cache remain unchanged. The upstream removal of the dedicated aggregate refusal does not add a second regex conversion or caching path. No update-specific complexity finding remains.
Which issue does this PR close?
No dedicated issue. Related to #4942, whose description says the remaining
Regex::newcalls were already hoisted into statics; these three user-pattern call sites were still compiling per batch on current main.Rationale for this change
regexp_extract,regexp_extract_all, andsplitcalledRegex::newon the user's pattern inside the per-batch evaluation path, so every 8192-row batch paid a full regex compile.rlikein the same crate already compiles once at plan time; these three could not take that exact shape because they are scalar functions created by name, and the pattern only arrives per invocation as a scalar argument.What changes are included in this PR?
Each planned expression now owns a one-slot
PatternCache(newstring_funcs/pattern_cache.rs): compile on first use, reuse while the pattern string is unchanged, recompile if it ever differs (split's serde does not require a literal pattern, so the cache tolerates changes rather than assuming a constant).Regexclones share the compiled program, so handing out clones per batch is an Arc bump. Error messages are byte-identical and an invalid pattern still fails at the same phase as before.Numbers on an M-series mac: criterion
regexp_extractgoes from 862us to 705us per 8192-row batch (about 18% faster), and a small-batch run (512 rows, 5000 batches) is 2.1x faster since compile cost is amortized over fewer rows. The split bench is flat because its case uses a literal delimiter, which takes the non-regex fast path. One known unknown worth stating: the cache uses a Mutex and the benches are single-threaded, so contention under DataFusion's intra-task parallelism is unmeasured. The fast path is a lock, a string compare, and a clone, so it should be negligible, and the lock also prevents duplicate compiles on a cold cache.How are these changes tested?
Seven new tests: three cache unit tests (compile-once, recompile-on-change, invalid pattern does not poison the slot), three multi-batch tests pinning one compile across batches per function via a test-only counter, and one pinning that an invalid split pattern still errors at evaluation. Full crate suites pass (670 spark-expr, 212 core), clippy with warnings denied and fmt are clean, and the Scala side was exercised through CometStringExpressionSuite (33 tests, includes the native split path) and CometRegExpJvmSuite (46 tests).