Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(cubesql): More complete usage of member_name_to_expr caching #8497

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

srh
Copy link
Member

@srh srh commented Jul 21, 2024

Makes column_name_to_member_vec do caching, removes some of its calls. But also, we replace a bunch of usage of find_member_by_alias_immutably with the mutable, caching version.

Most benchmarks do the same or better. long_simple_in_number_expr_1k is one which gets worse, at +9%. A few of the numbers:

power_bi_wrap                    -8.5660%
power_bi_sum_wrap                -9.1824%
long_in_expr                    -12.923%
long_simple_in_number_expr_1k    +9.1547%

The main source of worse performance would come from pre-emptively copying out and cloning vectors that were previously iterated in-place to avoid holding multiple mutable references to the EGraph. We could probably deal with that efficiently by using RefCell in the member_name_to_expr field, but we don't do that here.

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

@srh srh requested a review from a team as a code owner July 21, 2024 00:48
Copy link

codecov bot commented Jul 21, 2024

Codecov Report

Attention: Patch coverage is 97.49216% with 8 lines in your changes missing coverage. Please review.

Project coverage is 81.95%. Comparing base (858d965) to head (bb9e749).
Report is 1 commits behind head on master.

Files Patch % Lines
...besql/cubesql/src/compile/rewrite/rules/filters.rs 95.08% 6 Missing ⚠️
...besql/cubesql/src/compile/rewrite/rules/members.rs 99.08% 1 Missing ⚠️
...cubesql/cubesql/src/compile/rewrite/rules/order.rs 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8497      +/-   ##
==========================================
+ Coverage   81.93%   81.95%   +0.02%     
==========================================
  Files         199      199              
  Lines       76204    76323     +119     
==========================================
+ Hits        62435    62553     +118     
- Misses      13769    13770       +1     
Flag Coverage Δ
cubesql 81.95% <97.49%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@srh srh merged commit 3f369e3 into master Jul 24, 2024
67 checks passed
@srh srh deleted the column_name_to_member_vec-elimination branch July 24, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants