perf: optimize find_in_set (up to 24x faster) - #23460
Conversation
| // built at most once here rather than per row. | ||
| let map: Option<HashMap<&str, usize>> = | ||
| (str_list.len() >= FIND_IN_SET_LOOKUP_THRESHOLD).then(|| { | ||
| let mut map = HashMap::with_capacity(str_list.len()); |
There was a problem hiding this comment.
This map is reconstructed for each batch being processed. Would be better to reuse across batches.
There was a problem hiding this comment.
I looked into this, and it doesn't seem like it would improve performance much, and would be a significant change.
find_in_set (up to 24x faster)
| .iter() | ||
| .position(|s| *s == string) | ||
| .map_or(0, |idx| idx + 1); | ||
| let position = match &map { |
There was a problem hiding this comment.
should we pull the match on the map outside the loop or doesnt make too much of a difference?
There was a problem hiding this comment.
Good question — I benchmarked it. Hoisting the match into two loops is a tradeoff rather than a win: the map path gets ~5–7% faster (long_list_64 −5.0%, long_list_256 −7.2%), but the short-list linear-scan path regresses ~8% (short_list_4 +7.7%). Since lists below the threshold stay on the linear scan and short lists are the common case for find_in_set, I would rather not regress them for a gain on the path that is already HashMap-optimized. Keeping the single loop. Results were stable across three runs and two builds.
(Disclosure: I used an LLM-based coding assistant to run these benchmarks and help draft this reply.)
There was a problem hiding this comment.
kinda interesting how it causes a regression, woulda thought with less branching it at least be the same 🤔
| .iter() | ||
| .position(|s| *s == string) | ||
| .map_or(0, |idx| idx + 1); | ||
| let position = match &map { |
There was a problem hiding this comment.
kinda interesting how it causes a regression, woulda thought with less branching it at least be the same 🤔
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23460 +/- ##
==========================================
- Coverage 80.71% 80.71% -0.01%
==========================================
Files 1090 1090
Lines 369828 369866 +38
Branches 369828 369866 +38
==========================================
+ Hits 298493 298520 +27
- Misses 53548 53552 +4
- Partials 17787 17794 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| .iter() | ||
| .position(|s| *s == string) | ||
| .map_or(0, |idx| idx + 1); | ||
| let position = match &map { |
There was a problem hiding this comment.
a possible microoptimization if we can match through some bool expression, rather than Option
## Which issue does this PR close? N/A ## Rationale for this change Improve performance of existing expression. ## What changes are included in this PR? Replace per-row O(set_len) linear scan in find_in_set's constant-list path with a one-time HashMap lookup (threshold-guarded so short lists keep the linear scan), giving O(1) per-row probing for large sets. ## Are these changes tested? Existing tests + new tests Benchmark (criterion): - long_list_256: 95.827% faster (base 1246412ns -> cand 52009ns) - ~24x faster - short_list_4: 2.13% faster (base 64343ns -> cand 62972ns) - long_list_64: 88.562% faster (base 422083ns -> cand 48276ns) ## Are there any user-facing changes? No <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
Which issue does this PR close?
N/A
Rationale for this change
Improve performance of existing expression.
What changes are included in this PR?
Replace per-row O(set_len) linear scan in find_in_set's constant-list path with a one-time HashMap lookup (threshold-guarded so short lists keep the linear scan), giving O(1) per-row probing for large sets.
Are these changes tested?
Existing tests + new tests
Benchmark (criterion):
Are there any user-facing changes?
No