perf: optimize regexp_instr (40% faster)#23540
Conversation
andygrove
changed the title
regexp_instr (40% faster)
| }; | ||
| let pattern: &Regex = compile_and_cache_regex(pattern, flags, regex_cache)?; | ||
| // println!("get_index: value = {}, pattern = {}, start = {}, n = {}, subexpr = {}, flags = {:?}", value, pattern, start, n, subexpr, flags); | ||
| ) -> Result<i64, ArrowError> { |
There was a problem hiding this comment.
should it return -1 if there is no match? 0 sounds like legit index
| } | ||
|
|
||
| fn criterion_benchmark(c: &mut Criterion) { | ||
| let size = 1024; |
There was a problem hiding this comment.
I think most batches are 8k not 1k
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @andygrove -- this looks good to me
While reviewing I also found a few other cases that I think are worth testing; I made a separate PR for them here:
| use std::hint::black_box; | ||
| use std::sync::Arc; | ||
|
|
||
| fn create_args<O: OffsetSizeTrait>( |
There was a problem hiding this comment.
FWIW @andygrove -- it would make it easier to verify these results if you made one PR to add the benchmark and then a second PR to make the code change
That way we can use the run benchmark <benchmark> bot command to get a second performance run
| Ok(Some(match_start_char_offset)) | ||
| } else { | ||
| Ok(Some(0)) // Return 0 if the N-th match was not found | ||
| /// Compiles the patterns seen so far, keyed by `(pattern, flags)`. |
There was a problem hiding this comment.
this looks similar to https://github.com/apache/datafusion/blob/998f534aafbf55c83daaa6fd4985ba143954b0e0/datafusion/functions/src/regex/mod.rs#L131-L130
Maybe we could (as a follow on PR) extract the common regexp caching functionality) -- I personally prefer this Cache pattern -- for use in the other functions with regular expressions
3 participants
Which issue does this PR close?
N/A
Rationale for this change
Optimize existing expression.
What changes are included in this PR?
Removed per-batch default-array/Vec allocations, memoized the compiled regex across rows to skip HashMap hashing for literal patterns, and skipped the O(n) chars() scan on the default start=1 path.
Are these changes tested?
Existing tests.
Benchmark (criterion):
Full criterion output:
Are there any user-facing changes?
No