e-dard opened a new pull request #542: URL: https://github.com/apache/arrow-rs/pull/542
# Which issue does this PR close? N/A # 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 returned `DynComparator` for each row you are comparing the two arrays on. Due to the explicit lifetime of `DynComparator` I recently found it problematic to store the `DynComparator` 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%. ``` $ 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 ``` # What changes are included in this PR? I have refactored `DynComparator` to remove the explicit lifetime. I did this by removing the need for the `DynComparator` instantiations to close over references. Instead we just move in owned arrays by cheaply cloning the underlying `ArrayData`. Further, I took the liberty of making `DynComparator` `Send+Sync`. Again, this helps when you want to store it on types that need to be `Send + Sync`. Relative to how often the closure will be called I don't see this as an expensive change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
