Calculate Vec capacities in librustc #52438
Merged
Conversation
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
This comment has been minimized.
This comment has been minimized.
|
r? @estebank |
This comment has been minimized.
This comment has been minimized.
XAMPPRocky
added
the
S-waiting-on-review
|
Spurious error again; please retry. |
|
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.
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.) |
|
@nnethercote I added a short description (initially I just didn't think it was too informative); please advise if I'm missing anything. |
|
@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? |
|
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. |
|
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 |
|
@bors try Let's do a perf run I guess? |
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.
|
@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 |
|
💔 Test failed - status-travis |
|
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
|
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 |
|
I'll fix the error and do a perf run locally. |
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
0bb54a1 to
21956d3
Compare
|
Hmm, I can't build rust-perf due to I fixed the PR; @oli-obk can you try please? |
|
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 |
|
|
eddyb
reviewed
Jul 19, 2018
src/librustc/middle/dead.rs
Outdated
| { | ||
| let mut worklist = Vec::with_capacity(access_levels.map.len() + | ||
| if tcx.sess.entry_fn.borrow().is_some() { 1 } else { 0 } | ||
| ); |
There was a problem hiding this comment.
This can be created from an iterator and .collect().
eddyb
reviewed
Jul 19, 2018
src/librustc/infer/mod.rs
Outdated
| self.type_variables.borrow().num_vars() + | ||
| self.int_unification_table.borrow().len() + | ||
| self.float_unification_table.borrow().len() | ||
| ); |
There was a problem hiding this comment.
This can be created from an iterator and .collect().
eddyb
reviewed
Jul 19, 2018
src/librustc_driver/driver.rs
Outdated
| @@ -1353,7 +1353,8 @@ fn generated_output_paths( | |||
| exact_name: bool, | |||
| crate_name: &str, | |||
| ) -> Vec<PathBuf> { | |||
| let mut out_filenames = Vec::new(); | |||
| let mut out_filenames = Vec::with_capacity(sess.opts.output_types.len()); | |||
There was a problem hiding this comment.
This doesn't appear to be correct.
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.
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
src/librustc_typeck/astconv.rs
Outdated
| self.region_bounds.len() + | ||
| self.trait_bounds.len() + | ||
| self.projection_bounds.len() | ||
| ); |
There was a problem hiding this comment.
This can be created from an iterator and .collect().
|
In general we should really avoid |
|
@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. |
|
☀️ Test successful - status-travis |
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
340e1ee to
4978a48
Compare
|
@eddyb I addressed your comments, does it look good now? |
eddyb
reviewed
Jul 20, 2018
src/librustc/middle/dead.rs
Outdated
| { | ||
| let worklist = access_levels.map.iter().map(|(&id, _)| id).chain( | ||
| // Seed entry point | ||
| (*tcx.sess.entry_fn.borrow()).and_then(|(id, _, _)| Some(id)) |
There was a problem hiding this comment.
.and_then(|x| Some(y)) is always .map(|x| y) AFAIK
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
src/librustc_codegen_llvm/abi.rs
Outdated
| let args_capacity = self.args.iter().fold(0, |acc, arg| acc + | ||
| if arg.pad.is_some() { 1 } else { 0 } + | ||
| if let PassMode::Pair(_, _) = arg.mode { 2 } else { 1 } | ||
| ); |
eddyb
reviewed
Jul 20, 2018
src/librustc_typeck/astconv.rs
Outdated
| for projection in &self.projection_bounds { | ||
| vec.push(projection.to_predicate()); | ||
| } | ||
| Some(trait_ref.to_predicate()) |
There was a problem hiding this comment.
Same thing here, and_then can be just map.
|
@rust-timer build 435ce0bad1c0bc79ec8f0591eba110a92492e6ff |
|
Success: Queued 435ce0bad1c0bc79ec8f0591eba110a92492e6ff with parent 0fc4501, comparison URL. |
kennytm
added
the
S-waiting-on-perf
ljedrz
force-pushed
the
rustc_vec_capacity
branch
from
4978a48 to
8a29409
Compare
|
@eddyb done. |
eddyb
reviewed
Jul 20, 2018
src/librustc/middle/dead.rs
Outdated
| { | ||
| let worklist = access_levels.map.iter().map(|(&id, _)| id).chain( | ||
| // Seed entry point | ||
| (tcx.sess.entry_fn.borrow()).map(|(id, _, _)| id) |
There was a problem hiding this comment.
You don't need the parens either, sorry I wasn't clear before.
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
8a29409 to
e3d14c4
Compare
|
@bors r+ Thanks! |
|
📌 Commit e3d14c4 has been approved by |
bors
added
S-waiting-on-bors
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.
|
☀️ Test successful - status-appveyor, status-travis |
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.