Skip to content

Shrink Session a bit - #88530

Merged
bors merged 4 commits into
rust-lang:masterfrom
bjorn3:shrink_session
Sep 2, 2021
Merged

Shrink Session a bit#88530
bors merged 4 commits into
rust-lang:masterfrom
bjorn3:shrink_session

Conversation

@bjorn3

@bjorn3 bjorn3 commented Aug 31, 2021

Copy link
Copy Markdown
Member

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

rust-highfive commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rust-highfive

rust-highfive commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

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
Aaron1011
Aaron1011 reviewed Aug 31, 2021
View reviewed changes
Comment thread compiler/rustc_session/src/session.rs Outdated
cjgillot
cjgillot reviewed Aug 31, 2021
View reviewed changes
Comment thread compiler/rustc_codegen_cranelift/src/driver/aot.rs
cgu: &CodegenUnit<'_>,
work_products: &mut FxHashMap<WorkProductId, WorkProduct>,
) -> CompiledModule {
let incr_comp_session_dir = tcx.sess.incr_comp_session_dir();

@cjgillot cjgillot Aug 31, 2021

Copy link
Copy Markdown
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?

@bjorn3 bjorn3 Aug 31, 2021

Copy link
Copy Markdown
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

cjgillot commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Aug 31, 2021

Copy link
Copy Markdown
Collaborator

📌 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

GuillaumeGomez commented Sep 1, 2021

Copy link
Copy Markdown
Member

@bors rollup=never

@bors

bors commented Sep 1, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit b88ad42 with merge b778006db62a663595c8ffc810a20ddc85bf41b0...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

bors commented Sep 1, 2021

Copy link
Copy Markdown
Collaborator

💔 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

bjorn3 commented Sep 1, 2021

Copy link
Copy Markdown
Member Author

Spurious? This didn't touch rustdoc.

@GuillaumeGomez

GuillaumeGomez commented Sep 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

bjorn3 commented Sep 2, 2021

Copy link
Copy Markdown
Member Author

Rebased.

@bors r=cjgillot

@bors

bors commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator

📌 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

bors commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 74c7f12 with merge fcce644...

@bors

bors commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator

☀️ 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

pnkfelix commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

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

GuillaumeGomez commented Sep 8, 2021

Copy link
Copy Markdown
Member

Wild guess would be that it comes from this line.

@bjorn3 bjorn3 mentioned this pull request Sep 8, 2021
Merged
@bjorn3

bjorn3 commented Sep 8, 2021

Copy link
Copy Markdown
Member Author

#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

Mark-Simulacrum commented Oct 3, 2021

Copy link
Copy Markdown
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

@RalfJung RalfJung RalfJung left review comments
@cjgillot cjgillot cjgillot left review comments
+1 more reviewer
@Aaron1011 Aaron1011 Aaron1011 left review comments
Reviewers whose approvals may not affect merge requirements

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.