Remove ensure_done execution path

[Zoxc]

View all comments

This removes the ensure_done execution path as it doesn't have a performance impact. ensure_done calls are left as markers still.

[Zoxc]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0182a57 (0182a57e43b09ec52ae26ba9ba23c0af7f526148, parent: 69370dc4a8862b8401615a2a7b950704ba66c495)

[rust-timer]

Finished benchmarking commit (0182a57): comparison URL.

Overall result: ❌ regressions - 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

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 0.3%, secondary 1.2%)

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

	mean	range	count
Regressions ❌ (primary)	1.7%	[0.4%, 3.1%]	4
Regressions ❌ (secondary)	1.8%	[0.5%, 2.8%]	4
Improvements ✅ (primary)	-1.5%	[-2.7%, -0.8%]	3
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	0.3%	[-2.7%, 3.1%]	7

Cycles

Results (secondary 6.3%)

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)	6.3%	[5.9%, 6.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 482.035s -> 481.186s (-0.18%)
Artifact size: 397.04 MiB -> 397.03 MiB (-0.00%)

[Zoxc]

Should we remove ensure_done? It doesn't do much since we do cache promotion anyway. We could also leave it as marker in case we remove cache promotion. cc @nnethercote @Zalathar

[Zalathar]

I’m surprised that this doesn’t have a big perf impact, though as you say, that’s probably because we end up promoting the values from disk later on anyway.

The way that we currently do promotion seems like it has room for improvement, so it might be prudent to leave ensure_done intact for now, at least as a marker.

[nnethercote]

I'm always sympathetic to the idea of removing unnecessary stuff :) Is there a meaningful difference between tcx.ensure_done().foo() and let _ = tcx.foo()?

The comment on ensure_done says this:

    /// Wrapper that calls queries in a special "ensure done" mode, for callers
    /// that don't need the return value and just want to guarantee that the
    /// query won't be executed in the future, by executing it now if necessary.
    ///
    /// This is useful for queries that read from a [`Steal`] value, to ensure
    /// that they are executed before the query that will steal the value.
    ///
    /// Unlike [`Self::ensure_ok`], a query with all-green inputs will only be
    /// skipped if its return value is stored in the disk-cache. This is still
    /// more efficient than a regular query, because in that situation the
    /// return value doesn't necessarily need to be decoded.
    /// 
    /// (As with all query calls, execution is also skipped if the query result
    /// is already cached in memory.)

That sounds like partly an efficiency concern and partly a behavioural concern (the "guarantee" part). But I don't know if that comment is accurate. E.g. for the Steal part, I don't see much correlation between returning Steal and using ensure_done.

@Zoxc, do you know who introduced ensure_done? We should ask them.

[Zalathar]

The split between ensure_ok and ensure_done was introduced by #108820, though they had different names at the time. Previously, code would just use ensure_ok when it actually wanted ensure_done, leading to bugs when the provider was unexpectedly invoked later.

I introduced the current names and docs in #136279.

[nnethercote]

Ok, looks like @cjgillot and @oli-obk were involved (no surprises!), let's see if they have opinions.

[Zalathar]

E.g. for the Steal part, I don't see much correlation between returning Steal and using ensure_done.

In several (most?) cases, the query used with ensure_done does not return Steal itself, but does rely on some other query's return value not having been stolen yet.

Anything that inspects THIR or MIR has a reasonable chance of falling into this category, for example.

[oli-obk]

Yea I'd expect we'll need it for turning promoteds into their own items. But we can reintroduce this when needed

Just need to make sure we don't need it soon for https://github.com/rust-lang/rust/pull/153489/changes#diff-ad0c15bbde97a607d4758ec7eaf88248be5d6b8ae084dfc84127f81e3f7a9bb4R636

[Zoxc]

I've removed the ensure_done execution path, but kept the calls as markers. Hopefully this should fix #152662 as the current ensure_done execution path has race conditions.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, query-system
• compiler, query-system expanded to 69 candidates
• Random selection from 13 candidates

[oli-obk]

Curious. This removes 100% of cache hits of thir_abstract_const in various incremental patched tests I looked at. I don't know how to interpret that result, but it's not a perf issue anyway

[Zoxc]

check_if_ensure_can_skip_execution adds cache hits for tcx.ensure_done().thir_abstract_const when it marks the node green, that no longer occurs.

[nnethercote]

The comment on TyCtxt::ensure_done definitely needs updating.

More generally, making ensure_done a no-op but leaving all the calls is icky. Lots of potential for confusion!

View changes since this review

[Zalathar]

More generally, making ensure_done a no-op but leaving all the calls is icky. Lots of potential for confusion!

The question of whether to leave ensure_done in place as a trivial wrapper is a tricky one.

However, I do think it's ultimately useful to have an explicit marker for query calls that don't care about the return value, even if the marker currently (after this PR) makes no operational difference.

At the very least it's a lot more searchable than let _ = tcx.$query(..), and provides a place to document some reasons why callers might want to do this.

[nnethercote]

At the very least it's a lot more searchable than let _ = tcx.$query(..), and provides a place to document some reasons why callers might want to do this.

The problem is, how do we ensure that everyone uses ensure_done and not let _ = ...?

If we really wanted to distinguish queries were could do something like put a q_ prefix on their names. But when I suggested this recently on Zulip most people were opposed.

[nnethercote]

This should be renamed query_get_or_ensure_done. (I think the at can be dropped because it's low-value and QueryMode doesn't have it.)

[nnethercote]

The variants should be named GetOrEnsureDone and EnsureOk. (A bit clumsy but extremely clear.)

[Zalathar]

I think GetOrEnsureDone is more misleading.

It's not a get-or-ensure-done mode; it's a get mode that ensure-done delegates to because it currently doesn't have a specialized path of its own.

[nnethercote]

r? @Zalathar

[rustbot]

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

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