-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Remove unneeded macro witchery #23249
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
(rust_highfive has picked a reviewer for you, use r? to override) |
Does this have the same performance as the old version? (It certainly looks like it should, but that doesnt't mean that it actually does.) |
@huonw I don't know, but it should. The functions are marked as |
Yes, as I said that's the theory, but the practice may not match that. |
It'd be good to at least run a (can be out-of-tree) microbenchmark of say |
I'm consistently getting worse results for the new solution...
|
I don't think 12 MB/s is a reasonable number anyway, we need to examine that benchmark. |
With
The benchmark can be found here: https://gist.github.com/tbu-/c49e5a5ec4a4426b6d75 |
@tbu- are you perhaps forgetting to optimize the benchmark you gave? I'm getting some pretty wildly different results here:
|
@alexcrichton Indeed. I somehow assumed that So this seems safe to merge. |
@tbu- thanks for checking that! :) |
No description provided.