Remove queries from the driver interface #134302

bjorn3 · 2024-12-14T10:17:32Z

All uses of driver queries in the public api of rustc_driver have been removed in #134130 already. This removes driver queries from rustc_interface and does a couple of cleanups around TyCtxt construction and entering enabled by this removal.

Finishes the removal of driver queries started with #126834.

rustbot · 2024-12-14T10:17:41Z

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-12-14T10:17:43Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

bjorn3 · 2024-12-14T10:18:35Z

compiler/rustc_interface/src/passes.rs

+            &'tcx WorkerLocal<rustc_hir::Arena<'tcx>>,
+            F,
+        ) -> T,
+    > = Box::new(move |compiler, gcx_cell, arena, hir_arena, f| {


This is ugly, but the only way I could figure out to get all invariant lifetimes correctly inferred by rustc.

bjorn3 · 2024-12-14T10:18:56Z

compiler/rustc_interface/src/passes.rs

+
+                let res = f(tcx);
+                // FIXME maybe run finish even when a fatal error occured? or at least tcx.alloc_self_profile_query_strings()?
+                tcx.finish();


This fixme is a pre-existing issue.

compiler/rustc_driver_impl/Cargo.toml

This allows us to call GlobalCtxt::finish exactly once.

jieyouxu · 2024-12-15T05:29:55Z

r? jieyouxu

jieyouxu

Thanks, this looks very reasonable to me as a first-pass review. This is tricky enough that I want a second pair of eyes on it.

jieyouxu · 2024-12-15T15:05:50Z

r? @oli-obk (since you reviewed #134130)

oli-obk · 2024-12-15T17:22:47Z

Yea I already reviewed all of it but the lifetime stuff. But since at best I'd have some golfing for it, r=me,jieyouzu I can golf once it's landed

jieyouxu · 2024-12-15T17:51:20Z

In that case, I say we try to land this first, the lifetimes in passes are a bit annoying but I don't think is worth blocking, as oli said.

@bors r=oli-obk,jieyouxu rollup=never (tricky driver changes)

bors · 2024-12-15T17:51:23Z

📌 Commit b0cd37e has been approved by oli-obk,jieyouxu

It is now in the queue for this repository.

bjorn3 · 2024-12-16T11:30:48Z

@bors retry same spurious failure

bors · 2024-12-17T04:57:44Z

⌛ Testing commit b0cd37e with merge 978c659...

bors · 2024-12-17T07:40:27Z

☀️ Test successful - checks-actions
Approved by: oli-obk,jieyouxu
Pushing 978c659 to master...

rust-timer · 2024-12-17T08:58:36Z

Finished benchmarking commit (978c659): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.6%]	11
Regressions ❌ (secondary)	0.5%	[0.2%, 0.7%]	18
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.1%, 0.6%]	11

Max RSS (memory usage)

Results (primary 3.0%, secondary -1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.0%	[1.4%, 4.6%]	2
Regressions ❌ (secondary)	2.7%	[2.2%, 3.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.0%	[-9.9%, -2.4%]	6
All ❌✅ (primary)	3.0%	[1.4%, 4.6%]	2

Cycles

Results (primary 1.9%, secondary 3.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.0%, 3.2%]	6
Regressions ❌ (secondary)	3.1%	[1.5%, 9.9%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[1.0%, 3.2%]	6

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 773.739s -> 770.381s (-0.43%)
Artifact size: 332.93 MiB -> 330.98 MiB (-0.59%)

…eyouxu Improve dependency_format a bit * Make `DependencyList` an `IndexVec` rather than emulating one using a `Vec` (which was off-by-one as LOCAL_CRATE was intentionally skipped) * Update some comments for the fact that we now use `#[global_allocator]` rather than `extern crate alloc_system;`/`extern crate alloc_jemalloc;` for specifying which allocator to use. We still use a similar mechanism for the panic runtime, so refer to the panic runtime in those comments instead. * An unrelated refactor to `create_and_enter_global_ctxt` I forgot to include in rust-lang#134302. This refactor is too small to be worth it's own PR.

Rollup merge of rust-lang#134514 - bjorn3:more_driver_refactors, r=jieyouxu Improve dependency_format a bit * Make `DependencyList` an `IndexVec` rather than emulating one using a `Vec` (which was off-by-one as LOCAL_CRATE was intentionally skipped) * Update some comments for the fact that we now use `#[global_allocator]` rather than `extern crate alloc_system;`/`extern crate alloc_jemalloc;` for specifying which allocator to use. We still use a similar mechanism for the panic runtime, so refer to the panic runtime in those comments instead. * An unrelated refactor to `create_and_enter_global_ctxt` I forgot to include in rust-lang#134302. This refactor is too small to be worth it's own PR.

Improve dependency_format a bit * Make `DependencyList` an `IndexVec` rather than emulating one using a `Vec` (which was off-by-one as LOCAL_CRATE was intentionally skipped) * Update some comments for the fact that we now use `#[global_allocator]` rather than `extern crate alloc_system;`/`extern crate alloc_jemalloc;` for specifying which allocator to use. We still use a similar mechanism for the panic runtime, so refer to the panic runtime in those comments instead. * An unrelated refactor to `create_and_enter_global_ctxt` I forgot to include in rust-lang/rust#134302. This refactor is too small to be worth it's own PR.

Kobzol · 2024-12-23T13:52:06Z

This seems to have regressed doc builds (up to ~2.5% on primary benchmark doc builds). Was that expected?

bjorn3 · 2024-12-23T15:35:17Z

src/librustdoc/lib.rs

+        if sess.dcx().has_errors().is_some() {
+            sess.dcx().fatal("Compilation failed, aborting rustdoc");
+        }


Looks like this accidentally got moved out of the create_and_enter_global_ctxt().

Would this be related to the rustdoc regression?

I don't think so. In any case opened #135157 to revert this code movement.

bjorn3 · 2024-12-23T15:36:32Z

No regressions of any kind were expected.

Kobzol · 2024-12-23T22:26:46Z

Ok. Given that the regressions are tiny and only in doc builds, I think that it is fine.

@rustbot label: +perf-regression-triaged

…jieyouxu,GuillaumeGomez Move the has_errors check in rustdoc back to after TyCtxt is created This was accidentally moved before TyCtxt creation by rust-lang#134302.

Rollup merge of rust-lang#135157 - bjorn3:fix_rustdoc_error_abort, r=jieyouxu,GuillaumeGomez Move the has_errors check in rustdoc back to after TyCtxt is created This was accidentally moved before TyCtxt creation by rust-lang#134302.

bjorn3 added the A-driver Area: rustc_driver that ties everything together into the `rustc` compiler label Dec 14, 2024

rustbot assigned BoxyUwU Dec 14, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 14, 2024

bjorn3 commented Dec 14, 2024

View reviewed changes

klensy reviewed Dec 14, 2024

View reviewed changes

compiler/rustc_driver_impl/Cargo.toml Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bjorn3 added 5 commits December 14, 2024 14:24

Get rid of of the global_ctxt query

6ece803

Remove the parse query

8780253

Move GlobalCtxt::finish to TyCtxt

954cd79

This allows us to call GlobalCtxt::finish exactly once.

Immediately enter in TyCtxt::create_global_ctxt

6545a2d

Remove two unnecessary references

7738929

bjorn3 force-pushed the remove_driver_queries branch from a5188c2 to 7738929 Compare December 14, 2024 14:24

This comment has been minimized.

Sign in to view

Fix tests

b0cd37e

bjorn3 force-pushed the remove_driver_queries branch from 19bf2af to b0cd37e Compare December 14, 2024 14:54

rustbot assigned jieyouxu and unassigned BoxyUwU Dec 15, 2024

jieyouxu approved these changes Dec 15, 2024

View reviewed changes

rustbot assigned oli-obk and unassigned jieyouxu Dec 15, 2024

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 Dec 16, 2024

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 Dec 16, 2024

jieyouxu added CI-spurious-fail-msvc CI spurious failure: target env msvc and removed CI-spurious-fail-msvc CI spurious failure: target env msvc labels Dec 16, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2024

bors merged commit 978c659 into rust-lang:master Dec 17, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 17, 2024

rustbot added the perf-regression Performance regression. label Dec 17, 2024

bjorn3 deleted the remove_driver_queries branch December 17, 2024 12:43

bjorn3 mentioned this pull request Dec 19, 2024

Improve dependency_format a bit #134514

Merged

bjorn3 commented Dec 23, 2024

View reviewed changes

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 23, 2024

bjorn3 mentioned this pull request Jan 6, 2025

Move the has_errors check in rustdoc back to after TyCtxt is created #135157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove queries from the driver interface #134302

Remove queries from the driver interface #134302

bjorn3 commented Dec 14, 2024

rustbot commented Dec 14, 2024

rustbot commented Dec 14, 2024

bjorn3 Dec 14, 2024

bjorn3 Dec 14, 2024

This comment has been minimized.

This comment has been minimized.

jieyouxu commented Dec 15, 2024

jieyouxu left a comment •

edited

Loading

jieyouxu commented Dec 15, 2024

oli-obk commented Dec 15, 2024

jieyouxu commented Dec 15, 2024

bors commented Dec 15, 2024

bjorn3 commented Dec 16, 2024

bors commented Dec 17, 2024

bors commented Dec 17, 2024

rust-timer commented Dec 17, 2024

Kobzol commented Dec 23, 2024

bjorn3 Dec 23, 2024

lqd Dec 24, 2024

bjorn3 Jan 6, 2025

bjorn3 commented Dec 23, 2024

Kobzol commented Dec 23, 2024

Remove queries from the driver interface #134302

Remove queries from the driver interface #134302

Conversation

bjorn3 commented Dec 14, 2024

rustbot commented Dec 14, 2024

rustbot commented Dec 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jieyouxu commented Dec 15, 2024

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Dec 15, 2024

oli-obk commented Dec 15, 2024

jieyouxu commented Dec 15, 2024

bors commented Dec 15, 2024

bjorn3 commented Dec 16, 2024

bors commented Dec 17, 2024

bors commented Dec 17, 2024

rust-timer commented Dec 17, 2024

Overall result: ❌ regressions - please read the text below

Kobzol commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented Dec 23, 2024

Kobzol commented Dec 23, 2024

jieyouxu left a comment •

edited

Loading