Start cutting down rustc_query_system

[nnethercote]

The query system is implemented in rustc_query_system, rustc_middle, and rustc_query_impl. rustc_query_system is hamstrung by not having access to TyCtxt, and there seems to be consensus to eliminate it. It's contents can be moved into the other two crates. Moving as much stuff as possible to rustc_query_impl is preferred, because rustc_middle is already so big.

This PR starts this process. It moves one small function to rustc_middle and a good chunk of code to rustc_query_impl.

Once rustc_query_system is gone (or at least shrunk down a lot more) some of the traits like DepContext, QueryContext, and QueryDispatcher will be removable.

r? @Zalathar

[nnethercote]

cc @cjgillot @Zoxc

[Zoxc]

I'd prefer to apply the rustc_query_system::query::plumbing -> execution renaming in #151977 and then probably move the entire file, instead of growing rustc_query_impl's plumbing module.

[nnethercote]

You can't move the entire file. Some parts of it are used by rustc_middle, some aren't, so it needs to be split up accordingly.

[Zoxc]

Not enough moved for git to pick up a rename? It could be moved to a execution module in rustc_query_impl at least.

[Zalathar]

Adding a few hundred more lines to rustc_query_impl::plumbing is a bit unfortunate.

I'm not sure whether this will work out in practice, but here's a potential approach:

• Take the parts of rustc_query_system::query::plumbing that are not moving, and extract them into a separate module to remain in rustc_query_system.
• Move and rename rustc_query_system::query::plumbing, making it a submodule of rustc_query_impl::plumbing to minimize visibility headaches.

That would produce an end result similar to the current state of this PR, but with the migrated code still in its own separate file, and preserving as much line history as possible.

[nnethercote]

Adding a few hundred more lines to rustc_query_impl::plumbing is a bit unfortunate.

Why? The same few hundred lines are removed from rustc_query_system.

That would produce an end result similar to the current state of this PR, but with the migrated code still in its own separate file, and preserving as much line history as possible.

It would also result in an odd module structure that reflects historical evolution rather than what makes sense today. rustc_query_system is 5,000 lines of code. If we split it across two other modules there's going to be some churn. I am a firm believer that the current version of the code is the most important, and good code structure shouldn't be held hostage by concerns about version control history.

[Zalathar]

I tried prototyping my proposal, and while I still like it on paper, I came away with the distinct impression that it's probably not better enough to justify the extra fiddling and delay, compared to just moving forward with this PR.

Ultimately, what's most important is unlocking the sorts of simplification and improvement that are impossible with the current module split.

[Zalathar]

It would also result in an odd module structure that reflects historical evolution rather than what makes sense today. rustc_query_system is 5,000 lines of code. If we split it across two other modules there's going to be some churn. I am a firm believer that the current version of the code is the most important, and good code structure shouldn't be held hostage by concerns about version control history.

I do want to push back on this a bit.

Taking ~500 lines of code from system/plumbing and shoving it in impl/pluming is useful in the sense that it unlocks within-crate simplifications, but it's most certainly not a good module structure.

One of the first things I'd want to do is take big chunks of that code and extract it into one or more separate modules, so that it isn't making the impl/plumbing macros significantly harder to deal with.

And because the code has been isolated in a separate crate up to this point, it does represent a fairly “natural” boundary for the code that currently exists. IIRC, the main point of interaction between impl/plumbing and system/plumbing is through the get_query_incr and get_query_non_incr functions, with most of the rest of system/plumbing being internal details of those two public functions.

(For comparison, look at middle/plumbing, where I made a point of taking some functions that exist to be called by macro-generated functions, and moved them out to a separate inner submodule.)

So my main motivation for wanting to move the code over in a separate file is not to preserve history (though that's nice if we can get it), but because I think it's genuinely a better module structure for the current state of the code.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nnethercote]

My motivation is to get rid of all the different layers of indirection. I personally find that kind of thing much more of a barrier to understanding than module structure. I've added a new commit that starts to remove the indirection. It's nice!

[Zalathar]

The cleanups are nice, and it will be nice to unlock even more, so let’s try to get this into the next big rollup when the tree reopens.

@bors r+

[rust-bors]

📌 Commit d6218b6 has been approved by Zalathar

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[nnethercote]

I'd like to do a perf run before merging, just to be cautious.

@bors r-

[nnethercote]

@bors try @rust-timer queue

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

REPOSITORY                                   TAG       IMAGE ID       CREATED       SIZE
ghcr.io/dependabot/dependabot-updater-core   latest    bcec0b4e062b   10 days ago   783MB
=> Removing docker images...
Deleted Images:
untagged: ghcr.io/dependabot/dependabot-updater-core:latest
untagged: ghcr.io/dependabot/dependabot-updater-core@sha256:b662be51f7b8ef7e2c8464428f14e49cb79c36aa9afb7ecb9221dfe0f507050c
deleted: sha256:bcec0b4e062b5ffe11cc1c2729558c0cd96621c0271ab5e97ff3a56e0c25045a
deleted: sha256:64e147d5e54d9be8b8aa322e511cda02296eda4b8b8d063c6a314833aca50e29
deleted: sha256:5cba409bb463f4e7fa1a19f695450170422582c1bc7c0e934d893b4e5f558bc6
deleted: sha256:cddc6ebd344b0111eaab170ead1dfda24acdfe865ed8a12599a34d338fa8e28b
deleted: sha256:2412c3f334d79134573cd45e657fb6cc0abd75bef3881458b0d498d936545c8d
---
tests/ui/drop_non_drop.rs ... ok
tests/ui/drain_collect.fixed ... ok
tests/ui/duplicate_underscore_argument.rs ... ok
tests/ui/duplicated_attributes.rs ... ok
tests/ui/duration_suboptimal_units_days_weeks.rs ... ok
tests/ui/duration_suboptimal_units.rs ... ok
tests/ui/duration_subsec.rs ... ok
tests/ui/double_parens.fixed ... ok
tests/ui/duration_suboptimal_units_days_weeks.fixed ... ok
tests/ui/duration_suboptimal_units.fixed ... ok
tests/ui/duration_subsec.fixed ... ok
tests/ui/else_if_without_else.rs ... ok
tests/ui/eager_transmute.rs ... ok
tests/ui/elidable_lifetime_names.rs ... ok
tests/ui/empty_docs.rs ... ok
---
..............................................     (146/146)

======== tests/rustdoc-gui/search-filter.goml ========

[ERROR] line 48: Error: The CSS selector "#search-tabs .count.loading" still exists: for command `wait-for-false: "#search-tabs .count.loading"`
    at <file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html?search=test>

======== tests/rustdoc-gui/search-result-display.goml ========

[WARNING] line 39: Delta is 0 for "x", maybe try to use `compare-elements-position` instead?

[rust-bors]

☀️ Try build successful (CI)
Build commit: c175e8a (c175e8a23e88c960293fbd255990160a7e378823, parent: f889772d6500faebcac5bb70fa44b5e6581c38cd)

[rust-timer]

Finished benchmarking commit (c175e8a): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

Results (primary -1.0%, secondary -3.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-3.1%	[-4.4%, -1.8%]	2
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Cycles

Results (secondary -2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	10
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	10

Bootstrap: 472.84s -> 474.801s (0.41%)
Artifact size: 397.96 MiB -> 397.91 MiB (-0.01%)

[Zalathar]

I tried doing some work on a local branch on top of this one, and it was significantly harder, to the point that I was forced to pull the migrated code out into a separate module before I could try to get anything done.

It's so much harder to find anything or remain oriented in the file.

Shoving 500 lines of code into the bottom of rustc_query_impl::plumbing has made it a nightmare to work with.

[nnethercote]

I'm getting mixed messages. You complained, then gave r+, and now are complaining again.

I'm honestly surprised by what you're saying. Perhaps I'm insensitive to module structure. I find the presence/absence of modules within a crate to be a minor concern when it comes to understanding things. I find layers of abstraction and indirection like traits and generics much more of an impediment to understanding, especially when they cross crate boundaries. If you give me some clear suggestions I can try to work with them.

FWIW I have an unpublished follow-up that successfully removes the QueryDispatcher trait.

[Zalathar]

Sorry for the back-and-forth.

I had talked myself into approving the PR, despite my misgivings, because I had thought it was prudent to compromise and move forward, and because I didn’t want to ask you to redo a bunch of fiddly migration work.

Afterwards I tried working on top of this branch, excited to improve things further. But I quickly ran into serious navigation problems that were jarring enough to change my mind.

---

Despite its size, rustc_query_impl::plumbing has some structure to it that I rely on. The first half is an assortment of helper macros and modest helper functions, about 500 lines long. The second half is the big macro that consumes the query list, which itself is divided into a few different subsections.

Adding a large chunk of code at the bottom removes all the spatial landmarks I was using to find things in the big macro. And since it’s a macro, I have no other way to navigate it. I spent far too long trying to just find the macro portion in the larger file, as if it had vanished entirely.

I find it noticeably more difficult to work effectively in files of more than a few hundred lines, especially if I have to keep jumping back and forth between confusingly-named functions in different parts of the same file. That’s why I find it so helpful to split off coherent parts of large modules into separate files.

And while the code being moved from rustc_query_system::query::plumbing is not a perfectly coherent module, it is a decent approximation of one, making it a more useful navigational unit than a gigantic 1500-line soup.

[Zalathar]

Concretely, the main things I care about are:

• rustc_query_impl::plumbing should remain mostly intact, with no large insertions of code, and especially no large insertions at the bottom.
• The bulk of code being moved from rustc_query_system::query::plumbing should end up in its own separate file somewhere in _impl. I’m unsure whether it should be a submodule or a top-level module, but my guess right now is that top-level is preferable in the long run.
  • I don’t have a strong candidate for the module name, but the previous suggestion of execution seems decent enough.

(I would prefer to preserve as much line history as reasonably possible, because it makes a big practical difference in trying to understand the under-documented parts of the code. But I don’t know whether I can convince you of that, and I’d prefer to focus my energy on making the code nice enough for that to not matter so much in the long run.)

[nnethercote]

Ok, I'll redo it to put the moved code in its own module. I'll try to cajole git into preserving history.

When it comes to module/file size, 2,000 lines is when I start to feel uncomfortable. Clearly a matter of taste and personal preference! :)

[nnethercote]

I have an unpublished follow-up that successfully removes the QueryDispatcher trait.

That's now at #152215. It is rough quality and includes the commits from this PR. There are so many generics and trait bounds that it took quite some effort to get it to compile.