Query-ify global limit attribute handling

[Aaron1011]

Currently, we read various 'global limits' from inner attributes the crate root (recursion_limit, move_size_limit, type_length_limit, const_eval_limit). These limits are then stored in Sessions, allowing them to be access from a TyCtxt without registering a dependency on the crate root attributes.

This PR moves the calculation of these global limits behind queries, so that we properly track dependencies on crate root attributes. During the setup of macro expansion (before we've created a TyCtxt), we need to access the recursion limit, which is now done by directly calling into the code shared by the normal query implementations.

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit bfd7c7bb2b2a2c681a0c1ad00eee713d94e0010e with merge d606f2fd5e1ab800c6f9c463b1d637e2c9d884bf...

[bors]

☀️ Try build successful - checks-actions
Build commit: d606f2fd5e1ab800c6f9c463b1d637e2c9d884bf (d606f2fd5e1ab800c6f9c463b1d637e2c9d884bf)

[rust-timer]

Queued d606f2fd5e1ab800c6f9c463b1d637e2c9d884bf with parent 49ba936, future comparison URL.

[rust-timer]

Finished benchmarking try commit (d606f2fd5e1ab800c6f9c463b1d637e2c9d884bf): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

[rustbot]

Error: Label perf-regression can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

[Aaron1011]

r? @michaelwoerister

[michaelwoerister]

Thanks for the PR, @Aaron1011! I'll take a closer look shortly.

[michaelwoerister]

OK, overall this looks good to me. Getting things out of Session is almost always a good idea :)

Some remarks:

• I think the code would be a bit nicer if we only had a single global_limits query containing all the limits and then have helper methods on tcx to access the individual limits. In terms of dependency tracking granularity that should not make a difference. What do you think?
• There seems to be slight performance regression. I wonder if there is any way to get rid of that. I left comments in places where we might be able to cache the result of the query instead of re-invoking it many times.

[michaelwoerister]

@rylev, potential problem with performance regression bot above: #86674 (comment)

[rylev]

@michaelwoerister thanks! This has been fixed in #86687

[Aaron1011]

I think the code would be a bit nicer if we only had a single global_limits query containing all the limits and then have helper methods on tcx to access the individual limits. In terms of dependency tracking granularity that should not make a difference. What do you think?

I think the granularity could potentially make a difference - for example, changing the const_eval_limit attribute would cause us to re-run all evaluate_obligation queries, because of the dependency on recursion_limit in the combined limits struct.

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 9a6cd1a0d5a1db64b502d849edde2d9888f8b9ec with merge 27b275a44087f0a094822f466836c4503ba6a229...

[bors]

☀️ Try build successful - checks-actions
Build commit: 27b275a44087f0a094822f466836c4503ba6a229 (27b275a44087f0a094822f466836c4503ba6a229)

[rust-timer]

Finished benchmarking try commit (27b275a44087f0a094822f466836c4503ba6a229): comparison url.

Summary: This change led to significant improvements 🎉 in compiler performance.

• Moderate improvement in instruction counts (up to -1.1% on full builds of deep-vector-debug)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

[michaelwoerister]

Thanks for implementing the optimizations, @Aaron1011! I doesn't look like they made a huge difference but I think it's a good idea to have them anyway.

I think the granularity could potentially make a difference - for example, changing the const_eval_limit attribute would cause us to re-run all evaluate_obligation queries, because of the dependency on recursion_limit in the combined limits struct.

Yes, that's true. However, my thinking was that one very rarely adjusts those limits and thus this is a good trade-off between code readability and compile-time.
One scenario where merging the queries into a single one might actually improve performance is if some query X accesses multiple limits. It could make a performance difference if every invocation of X needs to add two or more edges in the dep-graph versus just a single one. It might make sense to give it a try since that overhead is a cost that is paid unconditionally; whereas the overly pessimistic cache invalidation would only have a negative effect if one of those limits actually changes in between compilation sessions. The performance numbers look like it's just the incremental test cases that regress while non-incremental ones are fine. Which suggests that we are dealing with dependency tracking overhead.

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 7e5a88a with merge 14d9c8245e32b898bc02bf215c498b682881f5d7...

[bors]

☀️ Try build successful - checks-actions
Build commit: 14d9c8245e32b898bc02bf215c498b682881f5d7 (14d9c8245e32b898bc02bf215c498b682881f5d7)

[rust-timer]

Queued 14d9c8245e32b898bc02bf215c498b682881f5d7 with parent 9044245, future comparison URL.

[rust-timer]

Finished benchmarking try commit (14d9c8245e32b898bc02bf215c498b682881f5d7): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

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

[michaelwoerister]

OK, bundling the queries together didn't seem to make a difference performance-wise. But I think the code looks cleaner at the usage sites this way.

Heads up performance triage: This PR introduces a slight regression in some incr. comp. cases because it fixes a dependency tracking hole and thus introduces some new tracking overhead. @Aaron1011 has already tried to optimize away some of that overhead as part of the PR, with limited effect unfortunately. But even if the overhead is here to stay, I think it is justified because it removes a potential source bugs.

Thanks for iterating on the PR, @Aaron1011!

@bors r+

[bors]

📌 Commit 7e5a88a has been approved by michaelwoerister

[bors]

⌛ Testing commit 7e5a88a with merge 969a6c2...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 969a6c2 to master...