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

rustc_metadata: Stop passing CrateMetadataRef by reference (step 1) #92277

Merged
merged 1 commit into from
Jan 14, 2022
Merged

rustc_metadata: Stop passing CrateMetadataRef by reference (step 1) #92277

merged 1 commit into from
Jan 14, 2022

Conversation

petrochenkov
Copy link
Contributor

It's already a (fat) reference.
Double referencing it creates lifetime issues for its methods that want to return iterators.

Extracted from #92245 for a perf run.
The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 25, 2021
@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 Dec 25, 2021
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 25, 2021
@bors
Copy link
Contributor

bors commented Dec 25, 2021

⌛ Trying commit d193a54cd1f2dfccfdb17a038fd4841963e51228 with merge a837562400f0ba930f9affd3c65842deabc06397...

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2021
@bors
Copy link
Contributor

bors commented Dec 25, 2021

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

@rust-timer
Copy link
Collaborator

Queued a837562400f0ba930f9affd3c65842deabc06397 with parent 67491a2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a837562400f0ba930f9affd3c65842deabc06397): comparison url.

Summary: This change led to small relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.4% on full builds of ctfe-stress-4)
  • Small regression in instruction counts (up to 0.8% on incr-unchanged builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 25, 2021
@petrochenkov petrochenkov changed the title rustc_metadata: Stop passing CrateMetadataRef by reference rustc_metadata: Stop passing CrateMetadataRef by reference (step 1) Dec 26, 2021
@petrochenkov petrochenkov mentioned this pull request Dec 26, 2021
Merged
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 26, 2021
@bors
Copy link
Contributor

bors commented Dec 26, 2021

⌛ Trying commit 89fa7ba70e72c500d66fe822265a583573da9351 with merge 6a9d84b4f802c371231707136ea41d0a9989b723...

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 26, 2021
@petrochenkov
Copy link
Contributor Author

@bors r-
This doesn't need a perf run.

The change caused a slight change in heuristics for inlining and codegen units (?) affecting a single stress test benchmark.
I think this can be merged as is.

Details:

  • The largest diff in the perf report is native_library_kind query.
  • From cg_diff it looks like after this PR <rustc_metadata::creader::CrateMetadataRef>::get_native_libraries got inlined in both rust/compiler/rustc_metadata/src/rmeta/decoder.rs and rust/library/core/src/iter/traits/iterator.rs codegen units.
  • From cg_diff it looks like after this PR rustc_metadata::rmeta::decoder::cstore_impl::provide_extern::native_libraries is no longer inlined in the rust/library/core/src/iter/traits/iterator.rs codegen unit.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 26, 2021
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Dec 26, 2021 •
edited
Loading

@bors try-

UPD: It doesn't work. Is there a way to cancel a try build?

@petrochenkov petrochenkov removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 26, 2021
@petrochenkov
Copy link
Contributor Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Jan 9, 2022

📌 Commit c4471b0 has been approved by jackh726

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 9, 2022
@matthiaskrgr
Rollup merge of rust-lang#92277 - petrochenkov:cmrval2, r=jackh726
cb3f5f8 
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1)

It's already a (fat) reference.
Double referencing it creates lifetime issues for its methods that want to return iterators.

---
Extracted from rust-lang#92245 for a perf run.
The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 9, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 10, 2022
@matthiaskrgr
Rollup merge of rust-lang#92277 - petrochenkov:cmrval2, r=jackh726
c663cb7 
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1)

It's already a (fat) reference.
Double referencing it creates lifetime issues for its methods that want to return iterators.

---
Extracted from rust-lang#92245 for a perf run.
The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 10, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 10, 2022
@matthiaskrgr
Rollup merge of rust-lang#92277 - petrochenkov:cmrval2, r=jackh726
423e355 
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1)

It's already a (fat) reference.
Double referencing it creates lifetime issues for its methods that want to return iterators.

---
Extracted from rust-lang#92245 for a perf run.
The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 10, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 12, 2022
@matthiaskrgr
Rollup merge of rust-lang#92277 - petrochenkov:cmrval2, r=jackh726
801a3b6 
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1)

It's already a (fat) reference.
Double referencing it creates lifetime issues for its methods that want to return iterators.

---
Extracted from rust-lang#92245 for a perf run.
The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 12, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 13, 2022
@matthiaskrgr
Rollup merge of rust-lang#92277 - petrochenkov:cmrval2, r=jackh726
bf9b7f9 
rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1)

It's already a (fat) reference.
Double referencing it creates lifetime issues for its methods that want to return iterators.

---
Extracted from rust-lang#92245 for a perf run.
The PR changes a lot of symbol names due to function signature changes, so it's hard to do differential profiling, let's spend some machine time instead.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 13, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 14, 2022
@bors
Auto merge of rust-lang#92844 - matthiaskrgr:rollup-z5wb6yi, r=matthi…
f312a5e 
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#90001 (Make rlib metadata strip works with MIPSr6 architecture)
 - rust-lang#91687 (rustdoc: do not emit tuple variant fields if none are documented)
 - rust-lang#91938 (Add `std::error::Report` type)
 - rust-lang#92006 (Welcome opaque types into the fold)
 - rust-lang#92142 ([code coverage] Fix missing dead code in modules that are never called)
 - rust-lang#92277 (rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1))
 - rust-lang#92334 (rustdoc: Preserve rendering of macro_rules matchers when possible)
 - rust-lang#92807 (Update cargo)
 - rust-lang#92832 (Update RELEASES for 1.58.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f84d35f into rust-lang:master Jan 14, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jackh726 jackh726

Labels
perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
7 participants
@petrochenkov @rust-highfive @rust-timer @bors @nagisa @jackh726 @rustbot