Skip to content
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

Eliminate some temporary vectors #77990

Merged
merged 5 commits into from
Nov 13, 2020
Merged

Eliminate some temporary vectors #77990

merged 5 commits into from
Nov 13, 2020

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 15, 2020

This PR changes get_item_attrs and get_item_variances to return iterator impls instead of vectors. On top of that, this PR replaces some seemingly unnecessary vectors with iterators or SmallVec, and also reserves space where we know (the minimum) number of elements that will be inserted. This change hopes to remove a few heap allocations and unnecessary copies.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2020
@Aaron1011
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 15, 2020

⌛ Trying commit d2e31209671acb9ffccf69ac9454b5b044059238 with merge 48c4cc18598e1e0dbfe4c5dd1fe08d184ee7659b...

@bors
Copy link
Contributor

bors commented Oct 15, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 48c4cc18598e1e0dbfe4c5dd1fe08d184ee7659b (48c4cc18598e1e0dbfe4c5dd1fe08d184ee7659b)

@rust-timer
Copy link
Collaborator

Queued 48c4cc18598e1e0dbfe4c5dd1fe08d184ee7659b with parent b5c9e24, future comparison URL.

@bugadani bugadani marked this pull request as draft October 15, 2020 22:36
@bugadani
Copy link
Contributor Author

I've converted this to draft for now. I want to collect some more of these changes and not litter github with multiple tiny PRs just for my vec-aversion.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (48c4cc18598e1e0dbfe4c5dd1fe08d184ee7659b): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@bugadani bugadani force-pushed the copies branch 3 times, most recently from 97dfb4d to 0919c43 Compare October 16, 2020 14:44
@bugadani bugadani marked this pull request as ready for review October 16, 2020 14:54
Cargo.lock Outdated Show resolved Hide resolved
@camelid camelid added the I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. label Oct 17, 2020
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation-wise, this looks good to me. I think it makes sense to trigger another perf run since you've pushed a bunch since the last one and I'm not sure if those changes included further optimizations and be likely to impact perf.

@bugadani
Copy link
Contributor Author

They shouldn't be hot I think, so unlikely, but I'd also prefer not merging this blindly.

@davidtwco
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 19, 2020

⌛ Trying commit a0471227b72c89271cde11e668e66422aff90aa7 with merge d6584ef1b48dbf79269fb72ae8709b509d1f2074...

@pietroalbini
Copy link
Member

Note: due to an infrastructure bug the try build will time out even though it completed. The underlying issue was fixed.

@bugadani bugadani requested a review from davidtwco October 19, 2020 22:26
@pietroalbini
Copy link
Member

@bors r- try- retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2020
@lcnr
Copy link
Contributor

lcnr commented Nov 8, 2020

lgtm

@bors r+

@bors
Copy link
Contributor

bors commented Nov 8, 2020

📌 Commit 6ed359c4aed753f0e988268ca477a5c6d741ee62 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2020
@bors
Copy link
Contributor

bors commented Nov 8, 2020

⌛ Testing commit 6ed359c4aed753f0e988268ca477a5c6d741ee62 with merge 9dea8044ddc86503ea16c07f1ec94c4411307100...

@bors
Copy link
Contributor

bors commented Nov 8, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2020
@bors
Copy link
Contributor

bors commented Nov 11, 2020

☔ The latest upstream changes (presumably #78825) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bugadani
Copy link
Contributor Author

@lcnr rebased

The test failure again looks like some apple specific delight.

------------------------------------------
stderr:
------------------------------------------
error: need to add support for DW_TAG_base_type '()' encoded with DW_ATE = 0x7, bit_size = 0
clang: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@lcnr
Copy link
Contributor

lcnr commented Nov 13, 2020

let's try again

@bors r+

@bors
Copy link
Contributor

bors commented Nov 13, 2020

📌 Commit f0d0d87 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2020
@bors
Copy link
Contributor

bors commented Nov 13, 2020

⌛ Testing commit f0d0d87 with merge a1f7ca7...

@bors
Copy link
Contributor

bors commented Nov 13, 2020

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing a1f7ca7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 13, 2020
@bors bors merged commit a1f7ca7 into rust-lang:master Nov 13, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 13, 2020
@bugadani bugadani deleted the copies branch November 13, 2020 15:15
nnethercote added a commit to nnethercote/perf-book that referenced this pull request Nov 19, 2020
@nnethercote
Copy link
Contributor

Nice changes. I have mentioned three of them as examples in the Rust Performance Book.

sigmaSd pushed a commit to sigmaSd/perf-book that referenced this pull request Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.