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

mark raw_vec::ptr with inline #79113

Merged
merged 1 commit into from
Jan 26, 2021
Merged

mark raw_vec::ptr with inline #79113

merged 1 commit into from
Jan 26, 2021

Conversation

andjo403
Copy link
Contributor

when a lot of vectors is used in a enum as in the example in #66617 if this function is not inlined and multiple cgus is used this results in huge compile times. with this fix the compile time is 6s from minutes for the example in #66617. I did not have the patience to wait for it to compile for more then 3 min.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Nov 16, 2020
@joshtriplett
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 17, 2020

⌛ Trying commit 88d1f31 with merge 35e31b36ab0b88482a02d490af328f51852bf292...

@bors
Copy link
Contributor

bors commented Nov 17, 2020

☀️ Try build successful - checks-actions
Build commit: 35e31b36ab0b88482a02d490af328f51852bf292 (35e31b36ab0b88482a02d490af328f51852bf292)

@rust-timer
Copy link
Collaborator

Queued 35e31b36ab0b88482a02d490af328f51852bf292 with parent f5230fb, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (35e31b36ab0b88482a02d490af328f51852bf292): 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
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@andjo403
Copy link
Contributor Author

a bit unfortunate increase for syn (what I can see that crate contains a lot of vec:s of the same type) but otherwise the perf looks ok.
I do not know how common the case to have large types with a lot of different vec:s like in #66617 is if that compile time degradation is ok or not.
but the compile time in the issue is improved many times compared with the 3% degradation for syn.

on the other hand this PR is maybe more a band-aid for this specific benchmark then a fix for the real problem.

@andjo403
Copy link
Contributor Author

@joshtriplett what do you think close or not?

@camelid camelid added T-libs Relevant to the library team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2020
@JohnCSimon JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2021
@andjo403 andjo403 closed this Jan 14, 2021
@m-ou-se m-ou-se reopened this Jan 25, 2021
@m-ou-se m-ou-se assigned m-ou-se and unassigned joshtriplett Jan 25, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jan 25, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 25, 2021

📌 Commit 88d1f31 has been approved by m-ou-se

@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 Jan 25, 2021
@jonas-schievink
Copy link
Contributor

@bors rollup=never

Has perf impact

@bors
Copy link
Contributor

bors commented Jan 26, 2021

⌛ Testing commit 88d1f31 with merge ff6ee2a...

@bors
Copy link
Contributor

bors commented Jan 26, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing ff6ee2a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 26, 2021
@bors bors merged commit ff6ee2a into rust-lang:master Jan 26, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 26, 2021
@andjo403 andjo403 deleted the raw_vec_ptr branch January 26, 2021 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants