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

Shrink Session a bit #88530

Merged
merged 4 commits into from
Sep 2, 2021
Merged

Shrink Session a bit #88530

merged 4 commits into from
Sep 2, 2021

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Aug 31, 2021

Remove a couple of unnecessary fields from Session and remove a Lock<T> for a field that is never mutated anyway.

@bjorn3 bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2021
@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rust-highfive
Copy link
Collaborator

r? @cjgillot

(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 Aug 31, 2021
cjgillot
cjgillot reviewed Aug 31, 2021
View reviewed changes
compiler/rustc_codegen_cranelift/src/driver/aot.rs
@@ -68,14 +68,13 @@ fn reuse_workproduct_for_cgu(
cgu: &CodegenUnit<'_>,
work_products: &mut FxHashMap<WorkProductId, WorkProduct>,
) -> CompiledModule {
let incr_comp_session_dir = tcx.sess.incr_comp_session_dir();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is something keeping the handling of the incr-comp session directory from being removed from Session?

Copy link
Member Author

Choose a reason for hiding this comment

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

It has to be stored somewhere. TyCtxt won't easily work as cg_ssa and cg_llvm require sess.incr_comp_session_dir() at a place where TyCtxt is not available. It is probably possible to refactor this away from Session and I was already looking into it a bit, but then I got distracted by refactorings to the crate loader.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2021

📌 Commit b88ad42 has been approved by cjgillot

@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 Aug 31, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 1, 2021
@GuillaumeGomez
Rollup merge of rust-lang#88530 - bjorn3:shrink_session, r=cjgillot
4f6211b 
Shrink Session a bit

Remove a couple of unnecessary fields from `Session` and remove a `Lock<T>` for a field that is never mutated anyway.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 1, 2021
Closed
@GuillaumeGomez
Copy link
Member

@bors rollup=never

@bors
Copy link
Contributor

bors commented Sep 1, 2021

⌛ Testing commit b88ad42 with merge b778006db62a663595c8ffc810a20ddc85bf41b0...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 1, 2021

💔 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 Sep 1, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 1, 2021

Spurious? This didn't touch rustdoc.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 2, 2021 •
edited
Loading

There are no flaky in those tests (they only check the generated HTML). Not sure what broke it in your PR though... Maybe rebase and try again?

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 2, 2021

Rebased.

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Sep 2, 2021

📌 Commit 74c7f12 has been approved by cjgillot

@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 Sep 2, 2021
@bors
Copy link
Contributor

bors commented Sep 2, 2021

⌛ Testing commit 74c7f12 with merge fcce644...

@bors
Copy link
Contributor

bors commented Sep 2, 2021

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing fcce644 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2021
@bors bors merged commit fcce644 into rust-lang:master Sep 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Sep 2, 2021
@bjorn3 bjorn3 deleted the shrink_session branch September 2, 2021 15:55
@pnkfelix
Copy link
Member

pnkfelix commented Sep 8, 2021

Oddly this was flagged as causing a large regression in instruction counts for deeply-nested-async benchmark during perf triage.

@pnkfelix pnkfelix added the perf-regression Performance regression. label Sep 8, 2021
@GuillaumeGomez
Copy link
Member

Wild guess would be that it comes from this line.

@bjorn3 bjorn3 mentioned this pull request Sep 8, 2021
Merged
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 8, 2021

#88748 will revert that change.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2021
@bors
Auto merge of rust-lang#88748 - bjorn3:try_fix_perf_regression, r=wes…
02a57fa 
…leywiser

Revert "Remove optimization_fuel_crate from Session"

This reverts commit 5464b2e.

This hopefully fixes the perf regression in rust-lang#88530 (comment).
@petrochenkov petrochenkov mentioned this pull request Sep 24, 2021
Open
@tmiasko tmiasko mentioned this pull request Sep 28, 2021
Merged
@Mark-Simulacrum
Copy link
Member

It turns out that the regression was in an unrelated area of the compiler and fixed after splitting the mir crate, before revert even landed. (#89322)

So, marking as triaged.

@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

@Aaron1011 Aaron1011 Aaron1011 left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@cjgillot cjgillot

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.56.0
Development

Successfully merging this pull request may close these issues.