Improve performance of strings count_matches utility with specific expression patterns - #22429
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Benchmark results show up to 25% improvement for and up to 21% for |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
📝 WalkthroughWalkthroughDetects when a regex can match an empty string, exposes that flag on device, adapts the counting kernel to select a positional policy accordingly, and adds a test validating empty-match counting for several patterns. ChangesEmpty Match Detection and Handling
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/strings/regex/regcomp.cpp`:
- Around line 1217-1243: The OR branch logic in reprog::compute_match_flags
(inside the check_paths lambda) incorrectly uses logical OR causing a
consuming-path result if either branch consumes; change the OR handling to
require both branches to be consuming by replacing the current `||` behavior
with logical AND so that the OR case returns self(self, inst.u2.left_id,
visited) && self(self, inst.u1.right_id, visited); this ensures patterns where
one branch reaches END without consuming (e.g., a*, a?, etc.) are detected as
empty-match paths.
In `@cpp/tests/strings/contains_tests.cpp`:
- Around line 411-415: The test loop is using the loop variable pattern but is
hardcoded to regex_program::create("a*"); change it so regex_program::create
uses the loop variable pattern (the variable named pattern) so each entry from
patterns is compiled and passed to cudf::strings::count_re(sv, *prog); ensure
you still compare *results to expected and keep the existing variables sv,
expected and function calls unchanged.
- Around line 408-410: The test currently hardcodes
cudf::strings::regex_program::create("a*") instead of using the loop variable
pattern, so change the call to use pattern (e.g.,
cudf::strings::regex_program::create(pattern)) in the contains test (where the
regex is constructed) and update expectations: for patterns that match empty
everywhere ("a*", "X?", "b{0,}", "()", "(?:)", "[a-z]*") keep expected
{6,6,1,4}; for "^" and "$" use {1,1,1,1}; for "^$" use {0,0,1,0}; and set
appropriate counts for "\\b" and "\\B" (or split the loop into separate
TEST_CASEs per pattern) so the assertion compares the result against the correct
per-pattern expected vector instead of a single hardcoded expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b6057e77-3a65-4626-876d-d6cc754e656f
📒 Files selected for processing (6)
cpp/src/strings/count_matches.cucpp/src/strings/regex/regcomp.cppcpp/src/strings/regex/regcomp.hcpp/src/strings/regex/regex.cuhcpp/src/strings/regex/regexec.cppcpp/tests/strings/contains_tests.cpp
|
/merge |
…pression patterns (NVIDIA#22429) Updates the logic in the `cudf::strings::detail::count_matches()` utility to use the faster `positional::END_ONLY` regex state engine path when input pattern does not include empty-matching instruction paths. The empty-matching-only instructions require tracking the start/end position which the `END_ONLY` path does not record. These patterns are not common like where the entire pattern is just an anchor like "\b" and "$" or the pattern can match virtually nothing (i.e. return an empty string) like with "(?:)" or "a*" or "a{0,}". The instruction set is analyzed for such a path and if it does not exist, the faster `END_ONLY` code logic is used. The `count_matches` utility is used in `count_re` as well as `split_re`, `findall` and `extract_all`. Authors: - David Wendt (https://github.com/davidwendt) - Gil Forsyth (https://github.com/gforsyth) Approvers: - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: NVIDIA#22429
Description
Updates the logic in the
cudf::strings::detail::count_matches()utility to use the fasterpositional::END_ONLYregex state engine path when input pattern does not include empty-matching instruction paths.The empty-matching-only instructions require tracking the start/end position which the
END_ONLYpath does not record. These patterns are not common like where the entire pattern is just an anchor like "\b" and "$" or the pattern can match virtually nothing (i.e. return an empty string) like with "(?:)" or "a*" or "a{0,}".The instruction set is analyzed for such a path and if it does not exist, the faster
END_ONLYcode logic is used.The
count_matchesutility is used incount_reas well assplit_re,findallandextract_all.Checklist