-
Notifications
You must be signed in to change notification settings - Fork 600
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
Any filter optimization #7205
Any filter optimization #7205
Conversation
Signed-off-by: Calum Murray <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7205 +/- ##
==========================================
- Coverage 77.54% 77.53% -0.02%
==========================================
Files 250 250
Lines 13529 13566 +37
==========================================
+ Hits 10491 10518 +27
- Misses 2514 2523 +9
- Partials 524 525 +1
☔ View full report in Codecov by Sentry. |
/hold |
Can you also show the memory consumption |
Signed-off-by: Calum Murray <[email protected]>
@pierDipi here is the benchmarks after the logic fix, and including the memory used for creating the filter and running the filter: Without Optimization
With Optimization
|
Signed-off-by: Calum Murray <[email protected]>
Note: with my most recent commit, I introduced a I've opened an issue for this here: #7210 |
The results of these optimizations are: Without Optimization
With Optimization
|
@pierDipi @matzew I marked this PR as ready to review as I think the optimizations I made are ready to be looked at, but there is probably a bit of work to do refactoring places where the filters are used before this is merged, so PR is on hold. Lmk if the performance tradeoffs here make sense to you, and I will work on those refactorings so that this can be merged! |
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Here is an updated comparison:
Note: the creation comparison is not one to one here, as in the new version of the code it is actually both creating and deleting the filter (as each creation creates a goroutine in the new version). Given that creation will only happen once (after #7213 is merged), I think we should move forwards with this option. |
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Calum Murray <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #7203
Proposed Changes
Release Note
Results
Before the optimization:
After the optimizations: