-
Notifications
You must be signed in to change notification settings - Fork 1k
Add a statically dispatched version of make_comparator
#8814
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
Conversation
dfc8cd3 to
6edfedf
Compare
|
Would be nice to see the sorting benchmarks on this change! My feeling is it might significantly reduce the overhead of the lexical sorting kernels (compared to row format). |
|
Or it might make it worse… it’s a trade off of indirection vs branch prediction. As we found out in apache/datafusion#18449 the same change can be 70% faster on ARM MacBooks and 70% slower on x86. So needless to say this should not be merged without benchmarks |
6edfedf to
01a89d7
Compare
Update all benchmark names from "comparator ..." to "make_comparator ..." to ensure compatibility with baseline comparisons across branches. This allows criterion to properly compare results between the add-comparator-benchmark and typed-comparator branches using --save-baseline and --baseline flags. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
I added benchmarks and comparing to main this seems overall slower: So I'm going to close this for now as a failed experiment. |
Working on apache/datafusion#18449 it came up that the dynamic dispatch here can hurt performance for simple types. This version seems to perform better in benchmarks there (admittedly run alongside other changes).