Ensure value is on the on-disk cache before returning from `ensure()`. #108820

cjgillot · 2023-03-06T15:41:37Z

The current logic for ensure() a query just checks that the node is green in the dependency graph.
However, a lot of places use ensure() to prevent the query from being called later. This is the case before stealing a query result.

If the query is actually green but the value is not available in the on-disk cache, ensure would return, but a subsequent call to the full query would run the code, and attempt to read from a stolen value.

This PR conforms the query system to the usage by checking whether the queried value is loadable from disk before returning.

Sadly, I can't manage to craft a proper test...

Should fix all instances of "attempted to read from stolen value".

rustbot · 2023-03-06T15:41:43Z

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

cjgillot · 2023-03-06T15:42:28Z

@bors try @rust-timer queue

bors · 2023-03-06T15:42:38Z

⌛ Trying commit add6a8420e4f417e4c82d159d91669eb053bb8a1 with merge 089156c8708f6aa6054e63459ce1726976130c3a...

bors · 2023-03-06T17:40:57Z

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

rust-timer · 2023-03-07T01:51:49Z

Finished benchmarking commit (089156c8708f6aa6054e63459ce1726976130c3a): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	75.7%	[1.4%, 201.4%]	121
Regressions ❌ (secondary)	56.1%	[0.2%, 633.1%]	88
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	73.3%	[-0.3%, 201.4%]	125

Max RSS (memory usage)

Results

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)	22.7%	[4.3%, 56.2%]	123
Regressions ❌ (secondary)	19.2%	[1.8%, 108.6%]	70
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	22.7%	[4.3%, 56.2%]	123

Cycles

Results

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)	85.7%	[1.2%, 188.6%]	122
Regressions ❌ (secondary)	67.3%	[1.4%, 592.2%]	76
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	85.7%	[1.2%, 188.6%]	122

rustbot · 2023-03-07T08:52:54Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot · 2023-03-07T08:54:04Z

@bors try @rust-timer queue

bors · 2023-03-07T08:54:15Z

⌛ Trying commit d71f87699257e1483f1058cb010c12b9c9c2e204 with merge c6f196f3b9487da4c5134007caf9c459ccea2f3e...

bors · 2023-03-07T11:02:27Z

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

rust-timer · 2023-03-07T12:21:10Z

Finished benchmarking commit (c6f196f3b9487da4c5134007caf9c459ccea2f3e): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to 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

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

Cycles

Results

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%	[2.7%, 3.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	3.0%	[2.7%, 3.2%]	2

oli-obk · 2023-03-07T12:32:14Z

compiler/rustc_middle/src/ty/query.rs

+    ///
+    /// This version verifies that the computed result exists in the cache before returning.
+    #[inline(always)]
+    pub fn ensure_with_value(self) -> TyCtxtEnsureWithValue<'tcx> {


What do you think about flipping the defaults just to nudge people in the right direction? So renaming ensure to ensure_without_checking_incremental_cache and renaming ensure_with_value to ensure.

There are currently 126 uses of ensure() in the codebase, and only <13 have a link with stealing a query result.
Most of them are in rustc_hir_analysis and rustc_interface as ways to drive analysis.

I can rename both to ensure_unchanged (current behaviour, to drive analyses) and ensure_with_value (new behaviour, for steal) to make everything explicit.

oof that's gonna be very verbose...

let's land this and I'll think about it a bit more

oli-obk · 2023-03-07T12:50:59Z

@rustbot author

r=me with CI happy

r? @oli-obk

cjgillot · 2023-03-11T22:46:25Z

@bors r=oli-obk

bors · 2023-03-11T22:46:27Z

📌 Commit e955ec0 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-03-12T14:00:31Z

⌛ Testing commit e955ec0 with merge f41927f...

bors · 2023-03-12T17:03:35Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing f41927f to master...

rust-timer · 2023-03-12T18:29:23Z

Finished benchmarking commit (f41927f): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.7%	[-3.7%, -3.7%]	1
Improvements ✅ (secondary)	-3.7%	[-5.1%, -2.3%]	2
All ❌✅ (primary)	-3.7%	[-3.7%, -3.7%]	1

Cycles

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

rustbot assigned TaKO8Ki Mar 6, 2023

cjgillot mentioned this pull request Mar 6, 2023

Fetch the value of has_ffi_unwind_calls before stealing mir_built. #108777

Closed

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 7, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 7, 2023

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Mar 7, 2023

oli-obk reviewed Mar 7, 2023

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2023

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 11, 2023

cjgillot added 4 commits March 11, 2023 22:41

Ensure value is on the on-disk cache before returning.

1ca103a

Use ensure for thir_abstract_const.

facf1e0

Make the check for cache opt-in.

382cc90

Use ensure_with_value in a few more places.

e955ec0

cjgillot force-pushed the ensure-on-disk branch from 7df1ad6 to e955ec0 Compare March 11, 2023 22:45

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2023

bors merged commit f41927f into rust-lang:master Mar 12, 2023

rustbot added this to the 1.70.0 milestone Mar 12, 2023

cjgillot deleted the ensure-on-disk branch March 12, 2023 17:05

This was referenced Mar 12, 2023

Enable -Zdrop-tracking-mir by default #107421

Merged

Move query accessor code into functions #107802

Closed

Check pattern refutability on THIR #108504

Merged

Use dynamic dispatch for queries #108638

Merged

cjgillot mentioned this pull request Mar 19, 2023

Error emitted for compiling tests #105157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure value is on the on-disk cache before returning from `ensure()`. #108820

Ensure value is on the on-disk cache before returning from `ensure()`. #108820

cjgillot commented Mar 6, 2023

rustbot commented Mar 6, 2023

cjgillot commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 7, 2023

rustbot commented Mar 7, 2023

cjgillot commented Mar 7, 2023

This comment has been minimized.

bors commented Mar 7, 2023

bors commented Mar 7, 2023

This comment has been minimized.

rust-timer commented Mar 7, 2023

oli-obk Mar 7, 2023

cjgillot Mar 7, 2023

oli-obk Mar 7, 2023

oli-obk commented Mar 7, 2023

cjgillot commented Mar 11, 2023

bors commented Mar 11, 2023

bors commented Mar 12, 2023

bors commented Mar 12, 2023

rust-timer commented Mar 12, 2023

Ensure value is on the on-disk cache before returning from ensure(). #108820

Ensure value is on the on-disk cache before returning from ensure(). #108820

Conversation

cjgillot commented Mar 6, 2023

rustbot commented Mar 6, 2023

cjgillot commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 7, 2023

Overall result: ❌ regressions - ACTION NEEDED

rustbot commented Mar 7, 2023

cjgillot commented Mar 7, 2023

This comment has been minimized.

bors commented Mar 7, 2023

bors commented Mar 7, 2023

This comment has been minimized.

rust-timer commented Mar 7, 2023

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 7, 2023

cjgillot commented Mar 11, 2023

bors commented Mar 11, 2023

bors commented Mar 12, 2023

bors commented Mar 12, 2023

rust-timer commented Mar 12, 2023

Overall result: ✅ improvements - no action needed

Ensure value is on the on-disk cache before returning from `ensure()`. #108820

Ensure value is on the on-disk cache before returning from `ensure()`. #108820