Remove queries from the driver interface by bjorn3 · Pull Request #134302 · rust-lang/rust

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

+ &'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

+
+ 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.

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.

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

+ 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.

marxin · 2025-01-10T06:20:25Z

@bjorn3
I noticed an orphan link in rustc-dev-guide:

error: Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_interface/queries/struct.Query.html
┌─ appendix/code-index.md:21:194
│
21 │ `Query` | struct | Represents the result of query to the `Compiler` interface and allows stealing, borrowing, and returning the results of compiler passes. | [The Rustc Driver and Interface] | [compiler/rustc_interface/src/queries.rs](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_interface/queries/struct.Query.html)
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_interface/queries/struct.Query.html

bjorn3 · 2025-01-20T15:37:31Z

That was fixed in rust-lang/rustc-dev-guide@7a4c3d3.

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.

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

Comment threadcompiler/rustc_driver_impl/Cargo.toml Outdated

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 7738929Compare 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 b0cd37eCompare 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

jieyouxu removed the CI-spurious-fail-msvc CI spurious failure: target env msvc label 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:masterDec 17, 2024

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

bjorn3 mentioned this pull request Jan 24, 2025
Internal Status Access Violation with constant redefinition #135870
Closed

Zoxc mentioned this pull request Mar 23, 2025
Find root cause why inlining GlobalCtxt::enter causes a crash in the deadlock handler #136499
Closed

Uh oh!

Uh oh!

Conversation

bjorn3 commented Dec 14, 2024

Uh oh!

rustbot commented Dec 14, 2024

Uh oh!

rustbot commented Dec 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

jieyouxu commented Dec 15, 2024

Uh oh!

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Dec 15, 2024

Uh oh!

oli-obk commented Dec 15, 2024

Uh oh!

jieyouxu commented Dec 15, 2024

Uh oh!

bors commented Dec 15, 2024

Uh oh!

bors commented Dec 17, 2024

Uh oh!

bors commented Dec 17, 2024

Uh oh!

rust-timer commented Dec 17, 2024

Overall result: ❌ regressions - please read the text below

Uh oh!

Kobzol commented Dec 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 commented Dec 23, 2024

Uh oh!

Kobzol commented Dec 23, 2024

Uh oh!

marxin commented Jan 10, 2025

Uh oh!

bjorn3 commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

jieyouxu left a comment •
edited
Loading