-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Optimize comparison functions of Iterator #45007
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
Replaced matching on tuples which led to less performant code generation.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Is improving the optimizer here a better idea than working around this specific problem? |
It'd depend on MIR optimizations, which consistently have not enough people working on them. |
I agree, improving the optimizer would be the ideal solution, but that's also more difficult to pull off. Is there any newbie friendly guide to adding optimizations to the MIR? I thinking we can rewrite match (x, y) {
(x_0, y_0) => b_{0, 0},
...
(x_m, y_n) => b_{m, n},
} into match x {
x_0 => match y { y_0 => b_{0, 0}, ..., y_n => b_{0, n}, },
...
x_m => match y { y_0 => b_{m, 0}, ..., y_n => b_{m, n}, },
} or whatever the equivalent is in MIR. This should also be generalizable to tuples of any length. The two transforms should be equivalent even if x and y have side effects since they are called in the same order (I'm assuming that the tuple arguments are evaluated from left-to-right, correct?), there are no other effects between their evaluation (the only thing in between is the pattern matching)**, and the they are called the same number of times. ** We could also evaluate them all first before any of the matching, but when I tested this, it seemed to produce worse code than when it was evaluated only right before the value was matched on. |
r? @bluss |
Thank you! @bors r+ |
📌 Commit 3264c83 has been approved by |
⌛ Testing commit 3264c83 with merge aac5ea781254d9d59adacc8937da9dd8e1912912... |
💔 Test failed - status-travis |
@bors retry
|
Optimize comparison functions of Iterator Replaced matching on tuples which led to less performant code generation. Testing on microbenchmarks consistently showed ~1.35x improvement in performance on my machine. Fixes #44729.
☀️ Test successful - status-appveyor, status-travis |
Replaced matching on tuples which led to less performant code generation. Testing on microbenchmarks consistently showed ~1.35x improvement in performance on my machine.
Fixes #44729.