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

[petrochenkov]

@Zoxc I'd like to merge this PR if it has a good chance to fix #152662.
Could you address the feedback above and rebase? Then I'll re-review and merge.
r? @petrochenkov

[Zoxc]

I don't think there was anything left to address here.

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

[petrochenkov]

@Zoxc could you set the S-waiting-on-review label when something is waiting on review, otherwise the status is not clear.
@rustbot ready

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit 218a075 has been approved by petrochenkov

It is now in the queue for this repository.

[petrochenkov]

@bors rollup=never

[rust-bors]

☀️ Test successful - CI
Approved by: petrochenkov
Duration: 3h 23m 23s
Pushing 42d7d52 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 5930afc (parent) -> 42d7d52 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 42d7d5292025a31aaf02100ae5bd664aca66bbbb --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. x86_64-gnu-gcc-core-tests: 8m 42s -> 14m 17s (+64.1%)
2. i686-gnu-nopt-1: 1h 40m -> 2h 25m (+44.3%)
3. dist-apple-various: 1h 25m -> 2h 2m (+44.0%)
4. x86_64-msvc-ext3: 1h 26m -> 1h 59m (+38.8%)
5. dist-x86_64-mingw: 2h 41m -> 1h 40m (-38.0%)
6. i686-msvc-2: 1h 44m -> 2h 19m (+34.2%)
7. dist-x86_64-musl: 1h 46m -> 2h 15m (+27.4%)
8. x86_64-msvc-1: 2h 53m -> 2h 7m (-26.6%)
9. x86_64-gnu-pre-stabilization: 30m -> 37m 32s (+25.1%)
10. dist-x86_64-illumos: 1h 50m -> 1h 24m (-23.7%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (42d7d52): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -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.3%, 0.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 2.5%)

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)	2.5%	[0.8%, 6.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.0%, secondary 4.1%)

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

	mean	range	count
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	7.3%	[6.5%, 8.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	2.0%	[2.0%, 2.0%]	1

Binary size

Results (secondary 0.0%)

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.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 518.611s -> 518.288s (-0.06%)
Artifact size: 401.49 MiB -> 401.47 MiB (-0.00%)