Repository navigation
Calculate Vec capacities in librustc - #52438
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
|
r? @estebank |
This comment has been minimized.
This comment has been minimized.
XAMPPRocky
added
the
S-waiting-on-review
Contributor
Author
|
Spurious error again; please retry. |
Member
|
Retried. cc @nnethercote -- some of these are iterating over the data that'll be inserted into the vector to calculate the length; do you know off-hand if that's a bad idea or not? |
This comment has been minimized.
This comment has been minimized.
Contributor
Not sure. If the data will be iterated over subsequently and it's not too large, it probably doesn't matter. Profiling is the ultimate test. (BTW, the PR lacks a description, as does the patch. That makes any kind of commentary more difficult. Even a single sentence of explanation would be helpful.) |
Contributor
Author
|
@nnethercote I added a short description (initially I just didn't think it was too informative); please advise if I'm missing anything. |
Contributor
|
@ljedrz: Often the "why" is more interesting than the "what", especially for PRs and commit messages. Do you have measurements showing that this speeds things up? |
Contributor
Author
|
I'd be happy to measure it; could you tell me which benchmarks I should run and how to do it? I couldn't find that information online. |
Contributor
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Contributor
|
@bors try Let's do a perf run I guess? |
Collaborator
bors
added a commit
that referenced
this pull request
Jul 17, 2018
Calculate Vec capacities in librustc Calculate the required capacity of a few vectors in rustc based on the number of elements they are populated with.
Contributor
|
@oli-obk started an online perf run for you, but you can also do local benchmarking and profiling. Documentation is here: https://github.com/rust-lang-nursery/rustc-perf/tree/master/collector What prompted you to write this patch? It adds some complexity, and generally you'd want to have some other clear benefit to compensate. The default growth strategy for |
Collaborator
|
💔 Test failed - status-travis |
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-author
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Contributor
Author
|
I'll fix the error and do a perf run locally. |
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
July 17, 2018 07:32
0bb54a1 to
21956d3
Compare
Contributor
Author
|
Hmm, I can't build rust-perf due to I fixed the PR; @oli-obk can you try please? |
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Contributor
Author
|
|
eddyb
reviewed
Jul 19, 2018
Contributor
There was a problem hiding this comment.
This can be created from an iterator and .collect().
eddyb
reviewed
Jul 19, 2018
Contributor
There was a problem hiding this comment.
This can be created from an iterator and .collect().
eddyb
reviewed
Jul 19, 2018
Contributor
There was a problem hiding this comment.
This doesn't appear to be correct.
Contributor
Author
There was a problem hiding this comment.
It's later shrunk at 1380; processing keys() before the loop (which was in my initial commit) caused a really uninformative error to occur during stage 1 build.
Contributor
There was a problem hiding this comment.
Then it's a better idea not to touch it at all. This is in the driver, and runs once per compilation, AFAICT, so it's not really worth optimizing (unless you can rewrite it to an iterator, that is).
eddyb
reviewed
Jul 19, 2018
Contributor
There was a problem hiding this comment.
This can be created from an iterator and .collect().
Contributor
|
In general we should really avoid |
Contributor
|
@eddyb do you want these to be refactored to iterators + collect even if perf shows no benefits? (With the alternative being to close this PR) Otherwise I say we do a perf run and see if there's any point to doing this at all. |
Collaborator
|
☀️ Test successful - status-travis |
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
July 20, 2018 14:35
340e1ee to
4978a48
Compare
Contributor
Author
|
@eddyb I addressed your comments, does it look good now? |
eddyb
reviewed
Jul 20, 2018
Contributor
There was a problem hiding this comment.
.and_then(|x| Some(y)) is always .map(|x| y) AFAIK
Contributor
There was a problem hiding this comment.
Oh and you shouldn't need the *, using by-value Option methods will do it for you.
eddyb
reviewed
Jul 20, 2018
eddyb
reviewed
Jul 20, 2018
Contributor
There was a problem hiding this comment.
Same thing here, and_then can be just map.
Member
|
@rust-timer build 435ce0bad1c0bc79ec8f0591eba110a92492e6ff |
Collaborator
|
Success: Queued 435ce0bad1c0bc79ec8f0591eba110a92492e6ff with parent 0fc4501, comparison URL. |
kennytm
added
the
S-waiting-on-perf
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
July 20, 2018 18:06
4978a48 to
8a29409
Compare
Contributor
Author
|
@eddyb done. |
eddyb
reviewed
Jul 20, 2018
Contributor
There was a problem hiding this comment.
You don't need the parens either, sorry I wasn't clear before.
Contributor
Author
There was a problem hiding this comment.
You're right, I should've noticed that; done.
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
July 20, 2018 20:02
8a29409 to
e3d14c4
Compare
Contributor
|
@bors r+ Thanks! |
Collaborator
|
📌 Commit e3d14c4 has been approved by |
bors
added
S-waiting-on-bors
Collaborator
bors
added a commit
that referenced
this pull request
Jul 21, 2018
Calculate Vec capacities in librustc Calculate the required capacity of a few vectors in rustc based on the number of elements they are populated with.
Collaborator
|
☀️ Test successful - status-appveyor, status-travis |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Calculate the required capacity of a few vectors in rustc based on the number of elements they are populated with.