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.

Sign up for GitHub

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

Jump to bottom

Code bloat from RawVec::grow_amortized #78471

Closed
glandium opened this issue Oct 28, 2020 · 4 comments · Fixed by #78682
Closed

Code bloat from RawVec::grow_amortized #78471

glandium opened this issue Oct 28, 2020 · 4 comments · Fixed by #78682
Labels
A-collections Area: `std::collection` I-heavy Issue: Problems and improvements with respect to binary size of generated code. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@glandium
Copy link
Contributor

The change in #72227 made non-PGO builds of Firefox's libxul.so's .text section grow by close to 1MB. libxul.so has a ~89MB section, which makes it a > 1% size increase in a library where about only 16% of the code is rust, which makes it a > 7% (!) increase in size. This comes from the fact that essentially all uses of Vec::push inline this all for some reason, and it's used all across the place.
@tesuji

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2020

Error: Label libs-impl can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@rustbot rustbot added I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 28, 2020
@jonas-schievink jonas-schievink added A-collections Area: `std::collection` T-libs Relevant to the library team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 28, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 28, 2020
@lcnr lcnr mentioned this issue Oct 28, 2020
Closed
@lcnr
Copy link
Contributor

lcnr commented Oct 28, 2020

inling grow_amortized seems to have a noticable performance benefit, so not doing so is probably not desirable, see #78483

Not sure how else we would be able to directly remedy this issue.

@spastorino
Copy link
Member

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@spastorino spastorino added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 28, 2020
@glandium glandium mentioned this issue Nov 2, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 15, 2020
@bors
Auto merge of rust-lang#78682 - glandium:issue78471, r=lcnr
e261649 
Do not inline finish_grow

Fixes rust-lang#78471.

Looking at libgkrust.a in Firefox, the sizes for the `gkrust.*.o` file is:
- 18584816 (text) 582418 (data) with unmodified master
- 17937659 (text) 582554 (data) with rust-lang#72227 reverted
- 17968228 (text) 582858 (data) with `#[inline(never)]` on `grow_amortized` and `grow_exact`, but that has some performance consequences
- 17927760 (text) 582322 (data) with this change

So in terms of size, at least in the case of Firefox, this patch more than undoes the regression. I don't think it should affect performance, but we'll see.
@bors bors closed this as completed in 76bd145 Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-collections Area: `std::collection` I-heavy Issue: Problems and improvements with respect to binary size of generated code. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not inline finish_grow glandium/rust
do not inline grow_amortized lcnr/rust
6 participants
@spastorino @glandium @jonas-schievink @tesuji @lcnr @rustbot