Disambiguate expansions relative to the creating DepNode #111815

cjgillot · 2023-05-21T10:01:03Z

Expansion data requires to be disambiguated in case of colliding spans. The current disambiguation mechanism was merely looking at hash collisions using a global disambiguation counter.

This global counter created an untracked data dependency between unrelated queries. For instance here: #110457 (comment)

This PR replaces the global disambiguation map by a per-DepNode map. This requires threading the currently evaluated DepNode through the implicit context. We use a dummy in the non-incremental case to simplify the logic.

rustbot · 2023-05-21T10:01:09Z

r? @wesleywiser

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

rustbot · 2023-05-21T10:01:13Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Noratrieb · 2023-05-21T10:34:52Z

compiler/rustc_middle/src/ty/context/tls.rs

@@ -31,6 +31,10 @@ pub struct ImplicitCtxt<'a, 'tcx> {
    /// Used to prevent queries from calling too deeply.
    pub query_depth: usize,

+    /// The DepNode of the query being executed. This is updated by the dep-graph. Thisis used to


Suggested change

/// The DepNode of the query being executed. This is updated by the dep-graph. Thisis used to

/// The DepNode of the query being executed. This is updated by the dep-graph. This is used to

oli-obk

this makes sense to me and the implementation does what you describe, but I'm far from fully grokking all the implications. I still think we should land it, unless you want to have someone else review it additionally.

oli-obk · 2023-05-22T15:23:01Z

compiler/rustc_middle/src/ty/context.rs

+    /// Fill an empty expansion. This method must not be used outside of the resolver.
+    #[inline]
+    #[instrument(level = "trace", skip(self))]
+    pub fn finalize_expansion(self, expn_id: LocalExpnId, expn_data: ExpnData) {
+        self.with_stable_hashing_context(|ctx| expn_id.set_untracked_expn_data(expn_data, ctx));
+    }


That seems like a very subtle invariant. Can it just be inlined into the resolver?

oli-obk · 2023-05-22T15:29:55Z

compiler/rustc_span/src/hygiene.rs

        HygieneData::with(|data| {
            let expn_id = data.local_expn_data.push(Some(expn_data));
            let _eid = data.local_expn_hashes.push(expn_hash);
            debug_assert_eq!(expn_id, _eid);
            let _old_id = data.expn_hash_to_expn_id.insert(expn_hash, expn_id.to_expn_id());
-            debug_assert!(_old_id.is_none());
+            if !_old_id.is_none() {


use is_some() and remove the _ from the variable name

cjgillot · 2023-05-24T16:51:14Z

As this touches the expansion infra, cc @petrochenkov

petrochenkov · 2023-05-24T19:10:13Z

I'm sufficiently sure that neither AST lowering nor especially MIR inlining should reuse the macro expansion machinery, just never got to investigating how to get rid of it.
I was never pinged on the relevant MIR change, so encountering ExpnKind::Inlined later was quite a WTF moment.

petrochenkov · 2023-05-24T19:14:23Z

I don't really understand how dep nodes work in this context, but assume the issue in #110457 happens because expansions are created "after incremental is enabled", and earlier expansions don't have such issues because they are still at the "redo everything and hash all the results" stage?

petrochenkov · 2023-05-24T19:27:51Z

compiler/rustc_span/src/lib.rs

@@ -2158,12 +2158,13 @@ where
        const TAG_INVALID_SPAN: u8 = 1;
        const TAG_RELATIVE_SPAN: u8 = 2;

+        // Hash expansion in all cases, even if we don't want to hash positions themselves.


petrochenkov · 2023-05-24T19:34:14Z

compiler/rustc_span/src/hygiene.rs

+            let _old_id = data.expn_hash_to_expn_id.insert(expn_hash, self.to_expn_id());
+            debug_assert!(_old_id.is_none());
+        });
+    }


Meta: code is changed and moved in the same commit, makes it harder to read the diff.

petrochenkov · 2023-05-24T19:36:13Z

compiler/rustc_query_system/src/query/plumbing.rs

+    debug!(
+        dep_node = ?tcx.dep_graph().data().unwrap().prev_node_of(prev_index),
+        ?new_hash, ?old_hash,
+    );


Nit: temporary debug output not removed (in some other places too).

petrochenkov · 2023-05-24T19:43:09Z

compiler/rustc_span/src/hygiene.rs

-    pub fn fresh_empty() -> LocalExpnId {
+    /// Create a new expansions without any information. This method must not be used outside of
+    /// the resolver.
+    pub fn reserve_expansion_id() -> LocalExpnId {


Suggested change

pub fn reserve_expansion_id() -> LocalExpnId {

pub fn reserve_expn_id() -> LocalExpnId {

Nit: it's pretty much always conventionally abbreviated in the current code.

petrochenkov · 2023-05-24T20:00:01Z

compiler/rustc_span/src/hygiene.rs

-pub struct FakeDepNode {
-    pub kind: u16,
-    pub hash: PackedFingerprint,
-}


There are some back and forth changes in the commits, probably makes sense to cleanup the history.

petrochenkov · 2023-05-24T20:05:48Z

compiler/rustc_span/src/hygiene.rs

@@ -1484,44 +1471,38 @@ impl<D: Decoder> Decodable<D> for SyntaxContext {
 #[instrument(level = "trace", skip(ctx), ret)]
 fn update_disambiguator(
    expn_data: &mut ExpnData,
-    dep_node: FakeDepNode,
+    hash_extra: impl Hash + Copy + fmt::Debug,


Suggested change

hash_extra: impl Hash + Copy + fmt::Debug,

hash_extra: impl Hash + Copy

Unused bound (in a couple of other places too).

petrochenkov · 2023-05-24T20:09:46Z

compiler/rustc_query_system/src/dep_graph/mod.rs

+    pub fn regular(dep_node: DepNode<K>) -> CurrentDepNode<K> {
+        CurrentDepNode::Regular {
+            dep_node,
+            expn_disambiguators: Lrc::new(Lock::new(UnhashMap::default())),


Suggested change

expn_disambiguators: Lrc::new(Lock::new(UnhashMap::default())),

expn_disambiguators: Default::default(),

Fingers crossed.

petrochenkov · 2023-05-24T20:15:15Z

compiler/rustc_query_impl/src/plumbing.rs

@@ -142,7 +142,7 @@ impl QueryContext for QueryCtxt<'_> {
                query: Some(token),
                diagnostics,
                query_depth: current_icx.query_depth + depth_limit as usize,
-                current_node: current_icx.current_node,
+                current_node: current_icx.current_node.clone(),


So there are two clones of the expansion disambiguation map and only one of them is updated.
Are changes to current_node.expn_disambiguators written back to current_icx.current_node.expn_disambiguators anywhere, or what is happening here?

oli-obk · 2023-05-24T20:45:36Z

I'm sufficiently sure that neither AST lowering nor especially MIR inlining should reuse the macro expansion machinery, just never got to investigating how to get rid of it.

Easy, we can just nuke it without replacement. The use case for it has since been refactored away, so at best it would cause a minor mir opt change

Remove ExpnKind::Inlined. Suggested in rust-lang#111815 (comment) r? `@oli-obk`

Remove ExpnKind::Inlined. Suggested in rust-lang#111815 (comment) r? ``@oli-obk``

bors · 2023-05-26T01:55:17Z

☔ The latest upstream changes (presumably #111960) made this pull request unmergeable. Please resolve the merge conflicts.

Remove ExpnKind::Inlined. Suggested in rust-lang/rust#111815 (comment) r? ``@oli-obk``

cjgillot · 2023-05-28T08:30:53Z

I don't really understand how dep nodes work in this context, but assume the issue in #110457 happens because expansions are created "after incremental is enabled", and earlier expansions don't have such issues because they are still at the "redo everything and hash all the results" stage?

Exactly.

The ICE #110457 itself has been fixed by #111952. The theoretical ICE still exists, but I don't think there is a way to trigger it right now.

This global disambiguation is a footgun for incremental, so I'm still proposing to land this PR.

Remove ExpnKind::Inlined. Suggested in rust-lang/rust#111815 (comment) r? ``@oli-obk``

petrochenkov · 2023-05-28T11:35:28Z

The initial idea was to create expansion disambiguators per parent DefPath (#49300 (comment)).
During actual implementation @Aaron1011 found that to be too expensive and came up with the current solution.

This solution is pretty similar to per-defpath disambiguators, right? Maybe with a slightly different granularity?

cjgillot · 2023-05-28T13:12:42Z

Yes, it's quite close.
We may not achieve the same level of hash-stability as using a per-DefPath disambiguator: all expansions created using macro expansion correspond to the resolver_for_lowering DepNode.
OTOH, this scheme ensures that we don't have data flowing across query boundaries, which is important for incremental soundness.

As the DefPath approach was limited by perf:
@bors try @rust-timer queue

bors · 2023-05-28T13:12:51Z

⌛ Trying commit 97a63ca with merge 516fd7002cc2ce4a8639ad243932a6e3a33a0847...

bors · 2023-05-28T15:25:21Z

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

rust-timer · 2023-05-28T17:39:43Z

Finished benchmarking commit (516fd7002cc2ce4a8639ad243932a6e3a33a0847): 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)	0.3%	[0.2%, 0.5%]	73
Regressions ❌ (secondary)	0.5%	[0.3%, 0.7%]	29
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.5%]	73

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

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.9%	[0.8%, 4.6%]	42
Regressions ❌ (secondary)	1.5%	[1.4%, 1.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[0.8%, 4.6%]	42

Binary size

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

Bootstrap: 647.057s -> 646.11s (-0.15%)

Zoxc · 2023-09-25T02:15:13Z

Could we limit the current disambiguation mechanism to just macro expansion say via. a scoped thread local?

I'd prefer to avoid extending the query system here, but I don't see any obviously wrong. It does seem this could use the query field of ImplicitCtxt instead of adding a new one, such that each query would have it's own counter.

cjgillot · 2023-09-26T18:15:04Z

This is not required any more, the bugs have been fixed independently. To be revisited if queries advance into the resolve.

petrochenkov · 2023-09-27T12:18:52Z

@cjgillot

This is not required any more, the bugs have been fixed independently.

In which PR exactly?
I try to follow changes that could be relevant, but didn't notice this one.

petrochenkov · 2023-09-27T12:19:57Z

#111950?
I assumed the issue existed for spans created during AST lowering too.

cjgillot · 2023-09-27T16:17:27Z

Both ExpnKind::Inlined (#111950) and DesugaringKind::Replace (#111952) have been removed, so we don't create new expansions after lowering.
We may have the issue with lowering if lowering stops being a monolithic query. For now, we should not reach it, as lowering gets reexecuted in each compilation.

rustbot assigned wesleywiser May 21, 2023

This comment was marked as outdated.

Sign in to view

cjgillot force-pushed the dep-node-disambiguate branch from c564776 to ffa197c Compare May 21, 2023 10:09

Noratrieb reviewed May 21, 2023

View reviewed changes

oli-obk approved these changes May 22, 2023

View reviewed changes

cjgillot force-pushed the dep-node-disambiguate branch 2 times, most recently from 15b6aa7 to 6a76425 Compare May 24, 2023 16:56

petrochenkov self-assigned this May 24, 2023

petrochenkov reviewed May 24, 2023

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2023

cjgillot mentioned this pull request May 25, 2023

Remove ExpnKind::Inlined. #111950

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 25, 2023

Rollup merge of rust-lang#111950 - cjgillot:expn-noinline, r=oli-obk

0390af0

Remove ExpnKind::Inlined. Suggested in rust-lang#111815 (comment) r? `@oli-obk`

compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 25, 2023

Rollup merge of rust-lang#111950 - cjgillot:expn-noinline, r=oli-obk

c2e3521

Remove ExpnKind::Inlined. Suggested in rust-lang#111815 (comment) r? ``@oli-obk``

saethlin pushed a commit to saethlin/miri that referenced this pull request May 26, 2023

Rollup merge of #111950 - cjgillot:expn-noinline, r=oli-obk

0c07f65

Remove ExpnKind::Inlined. Suggested in rust-lang/rust#111815 (comment) r? ``@oli-obk``

cjgillot added 4 commits May 28, 2023 08:04

Move expansion creation methods to TyCtxt.

4feb1b7

Track the invoking DepNode in the implicit context.

cdd8e9d

Disambiguate expansions per DepNode.

4f5db2c

Remove leading underscore.

bc333db

cjgillot added 5 commits May 28, 2023 08:04

Store disambiguators for each DepNode in CurrentDepNode.

6afc9dc

Store a reference instead of a Lrc.

067f1e2

Remove debugging line.

ffc0f5e

Abbreviate expansion.

959b228

Remove debugging.

7a2dc26

cjgillot force-pushed the dep-node-disambiguate branch from 6a76425 to 315d403 Compare May 28, 2023 08:22

This comment was marked as outdated.

Sign in to view

Use default for Lock.

97a63ca

cjgillot force-pushed the dep-node-disambiguate branch from 315d403 to 97a63ca Compare May 28, 2023 08:26

bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request May 28, 2023

Rollup merge of #111950 - cjgillot:expn-noinline, r=oli-obk

2f250c7

Remove ExpnKind::Inlined. Suggested in rust-lang/rust#111815 (comment) r? ``@oli-obk``

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 May 28, 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 May 28, 2023

cjgillot mentioned this pull request May 29, 2023

Incremental recompilation MIR ICE #110457

Closed

cjgillot closed this Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disambiguate expansions relative to the creating DepNode #111815

Disambiguate expansions relative to the creating DepNode #111815

cjgillot commented May 21, 2023 •

edited

Loading

rustbot commented May 21, 2023

rustbot commented May 21, 2023

This comment was marked as outdated.

Noratrieb May 21, 2023

oli-obk left a comment

oli-obk May 22, 2023

oli-obk May 22, 2023

cjgillot commented May 24, 2023

petrochenkov commented May 24, 2023

petrochenkov commented May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

petrochenkov May 24, 2023

oli-obk commented May 24, 2023

bors commented May 26, 2023

This comment was marked as outdated.

cjgillot commented May 28, 2023

petrochenkov commented May 28, 2023

cjgillot commented May 28, 2023

This comment has been minimized.

bors commented May 28, 2023

bors commented May 28, 2023

This comment has been minimized.

rust-timer commented May 28, 2023

Zoxc commented Sep 25, 2023

cjgillot commented Sep 26, 2023

petrochenkov commented Sep 27, 2023

petrochenkov commented Sep 27, 2023

cjgillot commented Sep 27, 2023

	/// The DepNode of the query being executed. This is updated by the dep-graph. Thisis used to
	/// The DepNode of the query being executed. This is updated by the dep-graph. This is used to

	pub fn reserve_expansion_id() -> LocalExpnId {
	pub fn reserve_expn_id() -> LocalExpnId {

	hash_extra: impl Hash + Copy + fmt::Debug,
	hash_extra: impl Hash + Copy

	expn_disambiguators: Lrc::new(Lock::new(UnhashMap::default())),
	expn_disambiguators: Default::default(),

Disambiguate expansions relative to the creating DepNode #111815

Disambiguate expansions relative to the creating DepNode #111815

Conversation

cjgillot commented May 21, 2023 • edited Loading

rustbot commented May 21, 2023

rustbot commented May 21, 2023

This comment was marked as outdated.

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented May 24, 2023

petrochenkov commented May 24, 2023

petrochenkov commented May 24, 2023

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

Choose a reason for hiding this comment

oli-obk commented May 24, 2023

bors commented May 26, 2023

This comment was marked as outdated.

cjgillot commented May 28, 2023

petrochenkov commented May 28, 2023

cjgillot commented May 28, 2023

This comment has been minimized.

bors commented May 28, 2023

bors commented May 28, 2023

This comment has been minimized.

rust-timer commented May 28, 2023

Overall result: ❌ regressions - ACTION NEEDED

Zoxc commented Sep 25, 2023

cjgillot commented Sep 26, 2023

petrochenkov commented Sep 27, 2023

petrochenkov commented Sep 27, 2023

cjgillot commented Sep 27, 2023

cjgillot commented May 21, 2023 •

edited

Loading