Optimize `incremental_verify_ich` #109371

Zoxc · 2023-03-19T23:59:42Z

This optimizes incremental_verify_ich by operating on SerializedDepNodeIndex, saving 2 hashmap lookups. The panic paths are also changed to get a TyCtxt reference using TLS.

rustbot · 2023-03-19T23:59:47Z

r? @jackh726

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

rustbot · 2023-03-20T00:30:50Z

These commits modify compiler targets.
(See the Target Tier Policy.)

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Noratrieb · 2023-03-20T06:05:08Z

@bors try @rust-timer queue

bors · 2023-03-20T06:05:18Z

⌛ Trying commit a00cf9280d48328a9f31c1f212d4f843b05ff5c4 with merge 057c6329ca8ff62e8e5c6a26d5fd1d6b7b6fbc12...

bors · 2023-03-20T08:06:52Z

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

rust-timer · 2023-03-20T10:11:55Z

Finished benchmarking commit (057c6329ca8ff62e8e5c6a26d5fd1d6b7b6fbc12): comparison URL.

Overall result: ✅ improvements - 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 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.8%	[-1.5%, -0.4%]	70
Improvements ✅ (secondary)	-1.4%	[-2.2%, -0.6%]	21
All ❌✅ (primary)	-0.8%	[-1.5%, -0.4%]	70

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.8%	[0.5%, 1.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.8%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.8%, 1.0%]	4

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)	-	-	0
Regressions ❌ (secondary)	3.8%	[3.0%, 4.6%]	2
Improvements ✅ (primary)	-0.6%	[-1.1%, -0.4%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-1.1%, -0.4%]	6

Zoxc · 2023-03-20T15:48:13Z

Let's do a perf run with DebugArg removed too, to see if anything interesting shows up.

cjgillot · 2023-03-20T17:07:52Z

@bors try @rust-timer queue

bors · 2023-03-20T17:08:01Z

⌛ Trying commit 2e7c50f62afbdf4fea28ea789d5d1a2d8ddee77f with merge 64c144e1a80f93ec678f258c8569a703deef71ad...

bors · 2023-03-20T19:04:25Z

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

bors · 2023-03-20T19:04:25Z

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

rust-timer · 2023-03-20T20:33:28Z

Finished benchmarking commit (64c144e1a80f93ec678f258c8569a703deef71ad): comparison URL.

Overall result: ✅ improvements - 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 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.8%	[-1.5%, -0.3%]	75
Improvements ✅ (secondary)	-1.3%	[-2.0%, -0.5%]	25
All ❌✅ (primary)	-0.8%	[-1.5%, -0.3%]	75

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.7%	[0.4%, 1.0%]	2
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-1.9%	[-3.8%, -0.4%]	6
Improvements ✅ (secondary)	-2.9%	[-4.4%, -1.4%]	52
All ❌✅ (primary)	-1.3%	[-3.8%, 1.0%]	8

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.7%, -0.5%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.7%, -0.5%]	3

cjgillot · 2023-03-21T12:03:47Z

compiler/rustc_query_system/src/dep_graph/graph.rs

-    pub fn is_green(&self, dep_node: &DepNode<K>) -> bool {
-        self.node_color(dep_node).map_or(false, |c| c.is_green())
+    #[inline]
+    pub fn is_index_green(&self, prev_index: SerializedDepNodeIndex) -> bool {


Can this still be called is_green, or is there a conflict?

It can. I just renamed it to contrast with the other is_green method.

cjgillot · 2023-03-21T12:05:07Z

compiler/rustc_query_system/src/dep_graph/graph.rs

-                let _current_fingerprint =
-                    crate::query::incremental_verify_ich(cx, data, result, &node, hash_result);
+            if let Some(prev_index) = data.previous.node_to_index_opt(&node)
+                && let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] }


Suggested change

&& let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] }

// Wrap the call to `lock()` in braces to unlock it before proceeding.

&& let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] }

You can't use braces like that to shorted lifetimes. I changed it to use nested ifs instead.

cjgillot · 2023-03-21T12:07:29Z

compiler/rustc_query_system/src/query/plumbing.rs

-            },
-        }
-    }
+    Tcx::with_context(|tcx| {


Can we just pass tcx as an argument to incremental_verify_ich_not_green?

Yes. I avoided passing it so it wouldn't affect the hot path. It's a quite minor improvement though.

cjgillot · 2023-03-21T12:08:10Z

compiler/rustc_query_system/src/query/plumbing.rs

 #[cold]
-fn incremental_verify_ich_failed(sess: &Session, dep_node: DebugArg<'_>, result: DebugArg<'_>) {
+#[inline(never)]
+fn incremental_verify_ich_failed<Tcx>(prev_index: SerializedDepNodeIndex, result: &dyn Debug)


Likewise, can we pass tcx here?

Zoxc · 2023-03-25T02:18:53Z

I changed it to pass tcx directly. We should do another perf run to verify that it doesn't regress anything.

jackh726 · 2023-03-25T02:51:13Z

@bors try @rust-timer queue

bors · 2023-03-25T02:51:22Z

⌛ Trying commit 820e3a8 with merge 223ac5351eb2e7ef0fc641afb06ec00d6fae10b5...

bors · 2023-03-25T04:45:59Z

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

rust-timer · 2023-03-25T06:03:30Z

Finished benchmarking commit (223ac5351eb2e7ef0fc641afb06ec00d6fae10b5): comparison URL.

Overall result: ✅ improvements - 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 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.9%	[-1.5%, -0.4%]	69
Improvements ✅ (secondary)	-1.1%	[-2.1%, -0.4%]	33
All ❌✅ (primary)	-0.9%	[-1.5%, -0.4%]	69

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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	1.1%	[0.5%, 1.9%]	5
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-0.9%	[-1.8%, -0.5%]	6
All ❌✅ (primary)	-0.8%	[-2.6%, 1.1%]	2

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)	1.7%	[1.5%, 2.0%]	3
Regressions ❌ (secondary)	0.9%	[0.6%, 1.5%]	11
Improvements ✅ (primary)	-1.7%	[-2.1%, -1.1%]	6
Improvements ✅ (secondary)	-1.2%	[-2.0%, -0.4%]	2
All ❌✅ (primary)	-0.6%	[-2.1%, 2.0%]	9

cjgillot · 2023-03-25T08:18:29Z

@bors r+

bors · 2023-03-25T08:18:31Z

📌 Commit 820e3a8 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-03-25T09:22:18Z

⌛ Testing commit 820e3a8 with merge e10eab5...

bors · 2023-03-25T11:54:40Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing e10eab5 to master...

bors · 2023-03-25T11:54:40Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing e10eab5 to master...

rust-timer · 2023-03-25T13:12:01Z

Finished benchmarking commit (e10eab5): 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.9%	[-1.6%, -0.4%]	78
Improvements ✅ (secondary)	-1.1%	[-2.2%, -0.5%]	39
All ❌✅ (primary)	-0.9%	[-1.6%, -0.4%]	78

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)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.1%	[-4.2%, -2.2%]	4
Improvements ✅ (secondary)	-2.4%	[-4.4%, -0.5%]	82
All ❌✅ (primary)	-2.0%	[-4.2%, 2.0%]	5

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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	0.9%	[0.4%, 3.6%]	11
Improvements ✅ (primary)	-0.9%	[-1.1%, -0.7%]	2
Improvements ✅ (secondary)	-0.9%	[-1.1%, -0.5%]	3
All ❌✅ (primary)	-0.2%	[-1.1%, 1.1%]	3

rustbot assigned jackh726 Mar 19, 2023

This comment has been minimized.

Sign in to view

Zoxc force-pushed the verify-hash-opt branch from 1fc40d6 to a6c7b38 Compare March 20, 2023 00:30

Zoxc force-pushed the verify-hash-opt branch from a6c7b38 to a00cf92 Compare March 20, 2023 00:31

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 20, 2023

This comment has been minimized.

Sign in to view

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

Zoxc force-pushed the verify-hash-opt branch from a00cf92 to 2e7c50f Compare March 20, 2023 15:45

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 20, 2023

cjgillot self-assigned this Mar 20, 2023

This comment has been minimized.

Sign in to view

cjgillot reviewed Mar 21, 2023

View reviewed changes

Zoxc added 4 commits March 25, 2023 02:59

Optimize incremental_verify_ich

8f29406

Remove DebugArg

dfae9c9

Split the if to release the lock earlier

afe4c16

Pass tcx directly

820e3a8

Zoxc force-pushed the verify-hash-opt branch from 2e7c50f to 820e3a8 Compare March 25, 2023 02:16

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 25, 2023

This comment has been minimized.

Sign in to view

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

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2023

bors added merged-by-bors This PR was explicitly merged by bors. labels Mar 25, 2023

bors merged commit e10eab5 into rust-lang:master Mar 25, 2023

rustbot added this to the 1.70.0 milestone Mar 25, 2023

Zoxc deleted the verify-hash-opt branch March 25, 2023 12:06

Noratrieb mentioned this pull request Mar 27, 2023

Very weird behavior when printing dep nodes #109676

Closed

	&& let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] }
	// Wrap the call to `lock()` in braces to unlock it before proceeding.
	&& let Some(dep_node_index) = { data.current.prev_index_to_index.lock()[prev_index] }

Optimize incremental_verify_ich #109371

Optimize incremental_verify_ich #109371

Conversation

Zoxc commented Mar 19, 2023

rustbot commented Mar 19, 2023

This comment has been minimized.

rustbot commented Mar 20, 2023

Noratrieb commented Mar 20, 2023

This comment has been minimized.

bors commented Mar 20, 2023

bors commented Mar 20, 2023

This comment has been minimized.

rust-timer commented Mar 20, 2023

Overall result: ✅ improvements - no action needed

Zoxc commented Mar 20, 2023

cjgillot commented Mar 20, 2023

This comment has been minimized.

bors commented Mar 20, 2023

bors commented Mar 20, 2023

bors commented Mar 20, 2023

This comment has been minimized.

rust-timer commented Mar 20, 2023

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zoxc commented Mar 25, 2023

jackh726 commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

bors commented Mar 25, 2023

This comment has been minimized.

rust-timer commented Mar 25, 2023

Overall result: ✅ improvements - no action needed

cjgillot commented Mar 25, 2023

bors commented Mar 25, 2023

bors commented Mar 25, 2023

bors commented Mar 25, 2023

bors commented Mar 25, 2023

rust-timer commented Mar 25, 2023

Overall result: ✅ improvements - no action needed

Optimize `incremental_verify_ich` #109371

Optimize `incremental_verify_ich` #109371