Replace `rustc_data_structures::thin_vec::ThinVec` with `thin_vec::ThinVec` by n...
source link: https://github.com/rust-lang/rust/pull/100869
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Contributor
nnethercote commented 17 days ago
rustc_data_structures::thin_vec::ThinVec
looks like this:
pub struct ThinVec<T>(Option<Box<Vec<T>>>);
It's just a zero word if the vector is empty, but requires two
allocations if it is non-empty. So it's only usable in cases where the
vector is empty most of the time.
This commit removes it in favour of thin_vec::ThinVec
, which is also
word-sized, but stores the length and capacity in the same allocation as
the elements. It's good in a wider variety of situation, e.g. in enum
variants where the vector is usually/always non-empty.
The commit also:
- Sorts some
Cargo.toml
dependency lists, to make additions easier. - Sorts some
use
item lists, to make additions easier. - Changes
clean_trait_ref_with_bindings
to take aThinVec<TypeBinding>
rather than a&[TypeBinding]
, because this
avoid some unnecessary allocations.
r? @spastorino
added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
labels
Collaborator
rustbot commented 17 days ago
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label
Contributor
Author
nnethercote commented 17 days ago
This comment has been minimized.
Contributor
Author
nnethercote commented 17 days ago
Collaborator
rust-timer commented 17 days ago
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
bors commented 17 days ago
Contributor
bors commented 17 days ago
Try build successful - checks-actions |
Collaborator
rust-timer commented 17 days ago
Collaborator
rust-timer commented 17 days ago
Finished benchmarking commit (ffb3c67): comparison URL. Overall result: regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesResults Footnotes
|
added perf-regression Performance regressions
and removed S-waiting-on-perf Status: Waiting on a perf run to be completed.
labels
Member
spastorino commented 17 days ago
@nnethercote I didn't check the code yet, just read your description but ... I was wondering if the perf results were the results you were expecting :). Can you comment on that?. |
Contributor
Author
nnethercote commented 17 days ago
Yes, this is disappointing. I'll have to see if I can optimize |
Contributor
Kobzol commented 17 days ago
I already tried this once in |
Contributor
Author
nnethercote commented 17 days ago
Is there a PR? |
(I already fixed a bug in |
Contributor
Kobzol commented 17 days ago
Hmm, I found #92514, but that's about thin slices, not thin vecs. I probably didn't even send a PR and just tested it locally, but I remember using the |
Contributor
Author
nnethercote commented 16 days ago
Hopefully Gankra/thin-vec#34 will fix most/all of the problems. |
Contributor
Author
nnethercote commented 16 days ago
Collaborator
rust-timer commented 16 days ago
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
bors commented 16 days ago
Contributor
Author
nnethercote commented 10 days ago
Collaborator
rust-timer commented 10 days ago
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
bors commented 10 days ago
Contributor
bors commented 10 days ago
Try build successful - checks-actions |
Collaborator
rust-timer commented 10 days ago
Collaborator
rust-timer commented 10 days ago
Finished benchmarking commit (ea65f30): comparison URL. Overall result: regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesResults Footnotes
|
removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
Author
nnethercote commented 10 days ago
@spastorino The improvements now slightly outweigh the regressions. I don't entirely trust the last set of results, because I think there is some noise in these results. But I think it's close enough to performance-neutral to proceed. |
Member
spastorino commented 10 days ago
@bors r+ |
Contributor
bors commented 10 days ago
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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Contributor
bors commented 7 days ago
Contributor
bors commented 7 days ago
Test successful - checks-actions |
Collaborator
rust-timer commented 7 days ago
Finished benchmarking commit (eac6c33): comparison URL. Overall result: regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesResults Footnotes
|
Contributor
Author
nnethercote commented 7 days ago
Ugh, the post-merge perf run looked very different to the pre-merge perf run, despite there being no change to the code. Clearly a lot of noise here, and hard to know which results to trust. I'll see if I can get any follow-up improvements. |
Contributor
rylev commented 2 days ago
@nnethercote any ideas on follow ups here? The results seem like they're partially noise, but some would appear to be real issues. |
Nothing right now, though I have a couple of experiments still to run. But |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
No reviews
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK