-
Notifications
You must be signed in to change notification settings - Fork 721
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
refactor: remove lifetime from DynComparator #542
Conversation
This commit removes the need for an explicit lifetime on the `DynComparator`. The rationale behind this change is that callers may wish to share this comparator amongst threads and the explicit lifetime makes this harder to achieve. As a nice side-effect, performance of the sort kernel seems to have improved: ``` $ critcmp master pr group master pr ----- ------ -- bool sort 2^12 1.03 310.8±1.34µs 1.00 302.8±7.78µs bool sort nulls 2^12 1.01 287.4±2.22µs 1.00 284.0±3.23µs sort 2^10 1.04 98.7±3.58µs 1.00 94.6±0.50µs sort 2^12 1.05 510.7±5.56µs 1.00 486.2±9.94µs sort 2^12 limit 10 1.05 48.1±0.38µs 1.00 45.6±0.30µs sort 2^12 limit 100 1.04 52.8±0.37µs 1.00 50.6±0.41µs sort 2^12 limit 1000 1.06 141.1±0.94µs 1.00 132.7±0.95µs sort 2^12 limit 2^12 1.03 501.2±4.01µs 1.00 486.5±4.87µs sort nulls 2^10 1.02 70.9±0.72µs 1.00 69.4±0.51µs sort nulls 2^12 1.02 369.7±3.51µs 1.00 363.0±18.52µs sort nulls 2^12 limit 10 1.01 70.6±1.22µs 1.00 70.0±1.27µs sort nulls 2^12 limit 100 1.00 71.7±0.82µs 1.00 71.8±1.60µs sort nulls 2^12 limit 1000 1.01 80.5±1.55µs 1.00 79.4±1.41µs sort nulls 2^12 limit 2^12 1.05 375.4±4.78µs 1.00 356.1±3.04µs ```
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.
Cool. Thanks, @e-dard .
I am not sure why there is this performance improvement, but I agree that cloning the array is cheap and give us the added bonus of Send+Sync
of the comparator that some use-cases benefit.
Could you add an issue and link it to this PR (via Closes #issue
), so that this shows on the changelog?
Note that this backward incompatible as code using the lifetime qualifier stops compiling.
I have filed #543 issue for this change |
Codecov Report
@@ Coverage Diff @@
## master #542 +/- ##
==========================================
+ Coverage 82.54% 82.60% +0.06%
==========================================
Files 167 167
Lines 45957 45984 +27
==========================================
+ Hits 37934 37985 +51
+ Misses 8023 7999 -24
Continue to review full report at Codecov.
|
Which issue does this PR close?
Closes #543
Rationale for this change
The cost of building a comparator (initialising a
DynComparator
) is often significantly higher than the actual cost of executing the comparator's closure on two row IDs. Therefore it makes sense to build the comparator once, and re-use the returnedDynComparator
for each row you are comparing the two arrays on.Due to the explicit lifetime of
DynComparator
I recently found it problematic to store theDynComparator
on an object that was used across threads in an async environment.By refactoring the
build_compare
implementations I was able to remove the lifetime and it's much easier to use in the Datafusion operator I'm improving.As a nice side-effect, the sort kernel benchmarks show an improvement in performance of around 2–5%.
What changes are included in this PR?
I have refactored
DynComparator
to remove the explicit lifetime. I did this by removing the need for theDynComparator
instantiations to close over references. Instead we just move in owned arrays by cheaply cloning the underlyingArrayData
.Further, I took the liberty of making
DynComparator
Send+Sync
. Again, this helps when you want to store it on types that need to beSend + Sync
.Relative to how often the closure will be called I don't see this as an expensive change.