Skip to content

GH-53159: [C++][Compute] Cache substring and regex matchers across invocations in scalar string kernels - #53160

Open
ishaaaan17 wants to merge 2 commits into
apache:mainfrom
ishaaaan17:perf/compute-cache-substring-matcher
Open

ishaaaan17 wants to merge 2 commits into
apache:mainfrom
ishaaaan17:perf/compute-cache-substring-matcher

Conversation

@ishaaaan17

@ishaaaan17 ishaaaan17 commented Oct 11, 2026 •

Copy link
Copy Markdown

rationale for this change

in cpp/src/arrow/compute/kernels/scalar_string_ascii.cc, the string matching kernels (match_substring, starts_with, ends_with, match_substring_regex and match_like) rebuild their matcher on every call to Exec. for regex matchers that means the RE2 DFA is recompiled for every batch in a stream or chunked array. plain matchers reallocate and rebuild the Knuth-Morris-Pratt prefix table each time.

this also closes out the existing todo in MatchSubstring:
// TODO Cache matcher across invocations (for regex compilation).

what changes are included in this PR?

  • replaced OptionsWrapper<MatchSubstringOptions> with a MatchSubstringState : public KernelState that caches the plain and regex matchers.
  • kept MatchSubstringState::Get(ctx) so other kernels and tests that use the old accessor still work.
  • PlainSubstringMatcher, PlainStartsWithMatcher, PlainEndsWithMatcher and RegexSubstringMatcher now reuse their compiled matchers across batches.
  • MatchLike looks at the SQL LIKE pattern and sets up the converter state once, instead of once per batch.
  • ran clang-format.

are these changes tested?

yes, by the existing kernel tests in cpp/src/arrow/compute/kernels/scalar_string_test.cc. they run the matching kernels over chunks, slices and scalars through CheckScalarUnary.

are there any user-facing changes?

no public api changes.

was ai used for this PR?

in line with the ai generation guidelines, here is how ai was used.

pr code and description written by:

  • human
  • ai

reviewed before submission by:

  • human
  • ai
  • not reviewed

i used an llm assistant to look at different ways of caching the matchers and to draft the first version of the changes. i then went through and reworked the design myself, checked that the old state accessors still work, made sure the options are stored safely, and formatted the code.

@github-actions github-actions Bot added the awaiting review Awaiting review label Oct 11, 2026
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #53159 has been automatically assigned in GitHub to PR creator.

@ishaaaan17

Copy link
Copy Markdown
Author

hi team,

a gentle reminder on this PR, whenever someone gets time, kindly approve the CI workflows.

i am happy to take any feedback on the design, change the caching approach, or share some more small benchmarks for reusing the string matcher across record batches, if you want. thanks!

return MatchSubstringImpl<Type, PlainSubstringMatcher>::Exec(ctx, batch, out,
matcher.get());
if (!state->plain_substring_matcher) {
ARROW_ASSIGN_OR_RAISE(state->plain_substring_matcher,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lazy initialization needs synchronization: Acero can evaluate the same bound expression concurrently across batches, sharing its kernel_state. Two threads can both observe an empty cache and assign different matchers, destroying a matcher while another thread is using it.

Please add a concurrent first-use regression test; sequential chunked-array tests would not catch this.

@lriggs do we already have a pattern to solve this?

@dmitry-chirkov-dremio

Copy link
Copy Markdown
Contributor

Could we add a focused Google Benchmark case in cpp/src/arrow/compute/kernels/scalar_string_benchmark.cc and share before/after results?

The existing helper calls CallFunction afresh each iteration, which recreates kernel state. To measure this optimization, please initialize one FunctionExecutor or bind one expression, then reuse it across multiple batches.

I'd suggest comparing:

  • One batch versus many batches, keeping the total input size fixed.
  • A simple and a more expensive regex, plain substring matching, and LIKE's fast path versus regex fallback.
  • Total execution including initialization, plus steady-state execution after the first batch.

A separate benchmark commit would make this easy to reproduce against both revisions. I'd prefer retaining a small representative benchmark for future regression tracking but I'm not sure about general policies of "bloating" the benchmarks.

p.s. please benchmark the version after fixing concurrent initialization (see my other comment)

@ishaaaan17

Copy link
Copy Markdown
Author

hi @dmitry-chirkov-dremio,

thank you very much for the detailed review, and for catching the race condition in Acero!

you are right. when the worker threads evaluate the same bound expression on different batches at the same time, they share kernel_state, and if the matcher is initialized on the first batch without any synchronization, it is a data race.

i will work on these:

  1. add synchronization for the matcher initialization. i can use a std::mutex with double-checked locking, or follow any specific pattern @lriggs suggests.
  2. add a unit test where multiple threads use a fresh kernel state for the first time at the same time.
  3. add the benchmark you asked for in scalar_string_benchmark.cc, comparing a single batch against reusing across multiple batches using FunctionExecutor, and share the before/after numbers.

thanks again for the guidance!

@ishaaaan17
ishaaaan17 force-pushed the perf/compute-cache-substring-matcher branch from 03c487c to e18a4e5 Compare October 16, 2026 19:36
@ishaaaan17

Copy link
Copy Markdown
Author

hi @dmitry-chirkov-dremio,

thanks for the detailed review and guidance! i have addressed all the points in two commits:

  1. thread-safe lazy initialization

    • fixed the race in MatchSubstringState using double-checked locking in GetOrInitMatcher, with std::atomic acquire/release and a std::mutex:
      • fast path (normal case across batches): only one atomic acquire load, no mutex contention.
      • slow path (first batch race): takes the mutex and does a relaxed double check before building and storing the matcher.
    • changed MatchLike to call ExecWithState(ctx, batch, out, like_state.get()) directly, so there is no ctx->SetState() change at runtime anymore.
    • fixed the formatting in enum class LikeKind so the clang-format CI check passes.
  2. concurrent first-use tests

    • added MatchSubstringConcurrentFirstUse and MatchLikeConcurrentFirstUse to TestBaseBinaryKernels in scalar_string_test.cc.
    • the test binds an expression that is not initialized yet, starts 16 threads that wait on an atomic barrier, and then all of them run ExecuteScalarExpression(bound_expr, batch) on the first batch together, to check there is no data race or use-after-free during initialization.
  3. multi-batch google benchmark

    • added MatcherReusedBenchmark in scalar_string_benchmark.cc, in a separate commit on top as you asked.
    • it uses GetFunctionExecutor with 1, 8 and 64 batches, keeping the total input fixed at 1M elements. it covers plain substring (match_substring), simple regex, expensive regex (match_substring_regex), the LIKE fast path, and the LIKE regex fallback (match_like).

@ishaaaan17

Copy link
Copy Markdown
Author

@ursabot please benchmark

@rok

rok commented Oct 16, 2026

Copy link
Copy Markdown
Member

Benchmark runs are scheduled for commit e18a4e5. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants