Improve regexp kernels performance by avoiding cloning Regex

[viirya]

Which issue does this PR close?

Closes #.

Rationale for this change

The regexp_match scalar expression in DataFusion is quite slow. As looking for the cause, it is found that the bad performance is due to cloning Regex per row. Regex has a CachePool. Cloning Regex will create a fresh CachePool and it is somehow expensive to re-creating cache space per row instead of reusing the cache.

What changes are included in this PR?

This patch removes cloning on Regex in regexp kernels.

regexp                  time:   [6.4153 ms 6.4331 ms 6.4519 ms]
                        change: [-97.721% -97.713% -97.705%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  8 (8.00%) high mild

Are there any user-facing changes?

[tustvold]

Perhaps you could use https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html#method.or_insert

[tustvold]

If you replaced the get above with this entry call, it might improve the performance further by eliminating a branch perhaps??

[viirya]

Hmm, then we need to use or_insert_with like

let re = patterns.entry(pattern).or_insert_with(|| {
  Regex::new(pattern.as_str()).map_err(|e| {
    ArrowError::ComputeError(format!(
      "Regular expression did not compile: {e:?}"
    ))
  }?)
});

But or_insert_with cannot propagate the error out of the function.

Tried with Result<Regex, ArrowError> as value type with the patterns HashMap, but then it doesn't work because the ? operator cannot be applied to type &mut Result<regex::Regex, arrow_schema::ArrowError> too.

So just keep it as is.

[tustvold]

Entry is an enumeration you can match on, you don't have to use or_insert_with and friends, they're just utilities like on Option

[viirya]

Oh, the branch to eliminate is the one inside or_insert? Do you mean something like:

let entry = patterns.entry(pattern.clone());
let re = match entry {
  Entry::Occupied(entry) => entry.into_mut(),
  Entry::Vacant(entry) => {
    let re = Regex::new(pattern.as_str()).map_err(|e| {
      ArrowError::ComputeError(format!(
        "Regular expression did not compile: {e:?}"
      ))
    })?;
    entry.insert(re)
  }
};

Hmm, it actually gets regression a little. 🤔

regexp                  time:   [7.6891 ms 7.7210 ms 7.7511 ms]
                        change: [+14.792% +15.570% +16.321%] (p = 0.00 < 0.05)
                        Performance has regressed.

[tustvold]

That is what I meant, perhaps the regression is because of the added clone (which i think can be removed)

[viirya]

clone? You mean let entry = patterns.entry(pattern.clone());? It cannot be removed.

216 |                 (Some(value), Some(pattern)) => {
    |                                    ------- move occurs because `pattern` has type `String`, which does not implement the `Copy` trait
217 |                     let entry = patterns.entry(pattern);
    |                                                ------- value moved here
...
221 |                             let re = Regex::new(pattern.as_str()).map_err(|e| {
    |                                                 ^^^^^^^ value borrowed here after move
    |
help: consider cloning the value if the performance cost is acceptable
    |
217 |                     let entry = patterns.entry(pattern.clone());
    |                                                       ++++++++

[tustvold]

Oh because of the unfortunate way flags are handled... Yeah this kernel would probably do with redesigning 😅

[tustvold]

As an aside it occurs to me we should probably have a version of this that takes Datum

[viirya]

As an aside it occurs to me we should probably have a version of this that takes Datum

Yea, I did a local version and tested it with DataFusion. It turns out the performance bottleneck is on cloning Regex mostly. But it is per query benchmark and I didn't run this kernel benchmark in it.

I will run this benchmark against the Datum version and probably submit it later.

[alamb]

I double checked that all the clone()s in arrow-string/src/regexp.rs are removed 👍

Thanks @viirya this is amazing

[Dandandan]

Thank you @viirya !

[viirya]

Thank you @alamb @Dandandan @tustvold for review