-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
Reserve capacity before extending Punctuated #1471
Conversation
I have done the following benchmark:
for _ in 0..10_000 {
std::hint::black_box(Punctuated::<u64, syn::token::Comma>::from_iter(0..4096));
std::hint::black_box(Punctuated::<u64, syn::token::Comma>::from_iter(
(0..4096).map(|i| syn::punctuated::Pair::Punctuated(i, Default::default())),
));
} Results:
This branch:
The speedup for this benchmark is about 2.5x :D I did get the same speedup for more |
I think that the assertions in |
While waiting for a review, I did just see that I could recycle Here is a benchmark: for _ in 0..1_000_000 {
let mut p: Punctuated<_, syn::token::Comma> = Punctuated::new();
for i in 0..16 {
p.push(i);
}
}
This branch:
Speedup about 1.9x |
Actually, why is |
@dtolnay Hey, is there something wrong with the PR? I know that it ended up a bit messy. I could create a new one, but I need some feedback first :) |
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.
I am pretty skeptical that the added complexity of all this is worthwhile. Is there a real use case that is sensitive to release-mode Punctuated::extend
performance? Could you give detail on what led you to pursue this particular change?
Thanks anyway for the PR!
@dtolnay I did read some of syn's source code while working on typed-builder and did find out that it can be optimized to avoid allocations. I did just remove some of Currently, More complexity could be avoided if I think that 2.5x speedup is worth it, but this is only my opinion. I wanted to add |
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.
Absent of a real use case where this change would make a difference, I prefer to keep the original straightforward code.
As a heavy user of Not sure how much this PR is going to help on our overall build times of course, but I might give it a shot soon to see if this PR improves build times for us. |
I tried recompiling our crates that heavily use both |
As you can probably tell, I am not surprised to hear that. A couple dozen memory allocations max per macro call is not a perceptible amount. If I had to guess, you are looking at an effect on the order of 0.0001% (1 microsecond per second spent on compilation). Any positive effect is reversed by 2 orders of magnitude by putting this amount of extra code into syn. The number of extra memory allocations from this new code during compiling syn would be 2 orders of magnitude bigger than it could possibly save downstream. |
@dtolnay Alright, thank you a lot for this explanation. I am still interested in knowing why Would it be fine for you if I would at least add |
While extending
Punctuated
from an iterator, usesize_hint
to reserve capacity before pushing to avoid repeated allocations.I did also avoid unneeded allocations of a new
Box
.There are also small optimizations like removing the check of
nomore
in the while loop and only checking if we did receive anEnd
.I did mention in a TODO comment that the behavior of
extend
from an iterator of pairs is not consistent with the behavior ofextend
from an iterator of items. You can also see it in the added test. I would recommend not adding a punctuation if the iterator is empty, but since that could be a braking change, I did not do it.This is my first contribution and I did not find a CONTRIBUTING file. Therefore, please tell me if something is odd :)
Sorry for the typo in the branch name :/