-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: Better GC and push_view for binviews #17627
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17627 +/- ##
==========================================
- Coverage 80.69% 80.64% -0.05%
==========================================
Files 1484 1485 +1
Lines 195409 195582 +173
Branches 2779 2782 +3
==========================================
+ Hits 157679 157728 +49
- Misses 37220 37342 +122
- Partials 510 512 +2 ☔ View full report in Codecov by Sentry. |
ced6f93
to
20e4883
Compare
These problems were caused by not deduplicating the data buffers. I make a refactor that moves buffer deduplication from |
Thank you @ruihe774. It looks good. I did a run of the mentioned regressions just to be sure and all is good. |
This PR add the logic to consider reference count when deciding whether to perform GC or not in
BinaryViewArrayGeneric::maybe_gc()
. Shared buffers that have multiple references won't be freed after GC, so including them when calculating memory savings, in the previous logic, does not make sense. This PR can avoid unnecessary GC in this case.