Add fast-path logic to some libcudf regex APIs - #22178
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. |
|
/ok to test |
|
/ok to test |
Adds a new libcudf API for counting string matches within rows of a strings column. This is meant to be a fast-path option for the current `cudf::strings::count()` which only accepts a regex pattern. If that API determines the pattern only contains a literal string (no regex pattern or flags) then this can be called internally automatically. Exposing this as a public API provides some consistency with other APIs like `find`, `contains`, and `split` which have both regex and non-regex versions. This API will match the behavior of the regex count API which includes not counting overlapping matches. Reference fast-path logic being worked on here: #22178 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #22288
bdice
left a comment
There was a problem hiding this comment.
Clear design, good solution.
cudf-spark correctness and performance validationI validated the current head Correctness blockerI found a reproducible semantic regression in the new Observed results: Under the existing cuDF behavior, This looks like a merge blocker. I recommend either preserving the before-final-newline behavior or excluding cudf-spark compatibilityThe current head completed the full cudf-spark Important scope note: cudf-spark passes Direct performanceMethod: 2,000,000 rows, 3 warmups, 9 measured samples, and three independent fresh JVMs per artifact. The values below are the median of the three per-JVM medians.
The shortest base kernels showed two GPU clock bands across processes, so the exact large speedup multipliers should not be treated as application-level Spark gains. The split result was stable across all three processes and consistently slower. My validation recommendation is do not merge the current head until the |
|
@wjxiz1992 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a ChangesRegex literal fast-path optimization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/include/cudf/strings/detail/find.hpp`:
- Around line 14-45: The cudf::strings::detail APIs in find.hpp are incorrectly
exposing default values for stream and mr, which should only be provided by
public APIs. Remove the default arguments from contains, starts_with, ends_with,
and count so these detail-layer functions match the coding guideline and the
pattern used in cudf::strings::detail::split. Keep the parameter order unchanged
with stream before mr, and update the declarations consistently across these
symbols.
In `@cpp/tests/strings/contains_tests.cpp`:
- Around line 806-814: The MULTILINE regex path is not being exercised because
`prog_ml` is created in the `contains_tests` block but the second
`cudf::strings::contains_re` call still uses `*prog` instead of `*prog_ml`.
Update the `contains_re` invocation tied to the `prog_ml =
cudf::strings::regex_program::create(...,
cudf::strings::regex_flags::MULTILINE)` setup so it passes `*prog_ml`, keeping
the expected assertions aligned with the multiline-specific behavior.
🪄 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: 10609d93-f6cc-4c9a-b221-447c304dbb0d
📒 Files selected for processing (12)
cpp/include/cudf/strings/detail/find.hppcpp/include/cudf/strings/detail/split.hppcpp/include/cudf/strings/regex/flags.hppcpp/include/cudf/strings/regex/regex_program.hppcpp/src/strings/contains.cucpp/src/strings/regex/regcomp.cppcpp/src/strings/regex/regcomp.hcpp/src/strings/regex/regex_program.cppcpp/src/strings/replace/replace_re.cucpp/src/strings/search/count.cucpp/src/strings/split/split_re.cucpp/tests/strings/contains_tests.cpp
lamarrr
left a comment
There was a problem hiding this comment.
This is really great, thanks for working on this!
|
@wjxiz1992 @mythrocks Could I get a spark review/approval? |
wjxiz1992
left a comment
There was a problem hiding this comment.
Retested the updated HEAD (b98ea54282351dbdc7696f8f2365a3e8359acf10). The correctness issue and the previously observed split regression are resolved.
Validation:
STRINGS_TEST: 536/536 passed.- End-anchor smoke passed:
abc$=true,true,true,trueabc\Z=false,true,false,true
- JNI/libcudf validation build:
BUILD SUCCESS.
Direct Java/JNI performance benchmark: 2,000,000 rows, 3 warmups, 9 measured samples, and 3 fresh JVMs per case. Values below are the median of the three per-JVM medians.
| Case | Previous base | Previous PR head | Updated PR head |
|---|---|---|---|
| contains literal | 2.110 ms | 0.813 ms | 0.828 ms |
contains ^literal |
4.217 ms | 0.180 ms | 0.185 ms |
contains literal$ |
4.742 ms | 0.160 ms | 3.154 ms |
| matches literal | 2.165 ms | 0.177 ms | 0.188 ms |
| replace literal | 16.885 ms | 8.462 ms | 8.577 ms |
| split literal | 23.443 ms | 29.440 ms | 23.727 ms |
The updated split result is 19.40% faster than the previous PR head and only 1.21% slower than the base, so the prior 25.58% split regression is no longer present. The updated literal$ result is within 0.20% of the previously measured correct control path (3.147949 ms); the old 0.160 ms result came from the incorrect end-anchor fast path.
Looks good to me.
|
@wjxiz1992 I had already removed the split fast-path since it showed a regression in cudf-spark. Our microbenchmarks showed a significant improvement so I would be grateful for a link to your split benchmark input for comparison and analysis. Meanwhile I will merge this as is since there are no more regressions and iterate specifically on split in follow up work. |
|
/merge |
|
@davidwendt Here is the complete benchmark source and the refreshed results: https://gist.github.com/wjxiz1992/9390e8e9a258cb5cb337b3b54a4ade7c One clarification: the 25.58% split regression was measured with a direct Java/JNI benchmark through the cuDF Java API, not as a Spark end-to-end timing. I used that layer to isolate the libcudf operation from Spark scheduling and aggregation noise. The exact split workload is:
Equivalent invocation after compiling the Gist source against the JNI JAR: REGEX_PERF_VARIANT=head \
java -Xms2g -Xmx8g -cp "$CUDF_JNI_JAR:$SLF4J_JAR:." \
RegexFastPathBenchmark split_literal 2000000 3 9The original comparison was 23.443 ms on base versus 29.440 ms on the earlier PR head. After the split fast path was removed, I measured 23.727 ms on |
Description
Adds fast-path operations to some of the
xx_re()APIs where depending on the pattern, a more efficient algorithm may be used. For example,contains_re()can call regularcontains()for literals andstarts_with()orends_with()appropriately if the regex pattern indicates.Checklist