Use per-parent disambiguators everywhere

[aerooneqq]

View all comments

This PR addressing the following concerns about per-parent disambiguators (#153955):

• DisambiguatorState is removed, PerParentDisambiguatorState is now used everywhere,
• Steals were removed from every per-parent disambiguator in resolver,
• It adds parent field in PerParentDisambiguatorState in #[cfg(debug_assertions)] for asserting that per-parent disambiguator corresponds to the same LocalDefId which is passed into create_def,
• Removes Disambiguator trait replacing it with Disambiguator enum, with this change we no longer expose next method (as trait should be public otherwise the warning will be emitted). It may affect perf in a negative way though.

Those changes should not fix perf issues that were reported, perf run that was attempted before showed much better results. Performance can be probably fixed by removing per-parent disambiguators replacing them with a single one as it was before, then it will be passed to AST -> HIR lowering and modified. For delayed owners we can store followup disambiguators as it was in the beginning of the #153955 per-parent disambiguators. This solution should save achievements from #153955 (removed DefPathData variants).
However, I would prefer to keep per-parent disambiguators as it seems a better architectural solution for me.

r? @petrochenkov
cc @oli-obk

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[oli-obk]

per parent logic is definitely the right thing, and we should take small hits now to clean all that up.

[aerooneqq]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

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

[oli-obk]

just a nit, then lgtm

View changes since this review

[aerooneqq]

@rustbot ready

Lets try run perf, a lot of things have changed.

[oli-obk]

@bors r+ rollup

View changes since this review

[rust-bors]

📌 Commit 3ed10c2 has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

@bors try @rust-timer queue

The changes seemed innocent enough, but yea, there have been surprises in this are before

[petrochenkov]

(Please also squash commits before merging.)

[petrochenkov]

This looks sort of messy now compared to what we have on master, I guess it's the cost of using PerParentDisambiguatorState everywhere and avoiding Steal.

[oli-obk]

This looks sort of messy now compared to what we have on master, I guess it's the cost of using PerParentDisambiguatorState everywhere and avoiding Steal.

I think it should get better once I do the cleanups mentioned earlier in this PR.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 07b9436 (07b94367b014d301ae457797f418cb5d79526b42, parent: 9ab01ae53c416f89fe256b79588a76dcbcdc9290)

[rust-timer]

Finished benchmarking commit (07b9436): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

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

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.7%, secondary 1.7%)

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

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

Cycles

Results (primary 3.1%, secondary 3.9%)

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

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.352s -> 489.481s (-0.18%)
Artifact size: 394.40 MiB -> 394.42 MiB (0.01%)

[aerooneqq]

@rustbot ready

[oli-obk]

@bors r+ rollup

[rust-bors]

📌 Commit 2e6a082 has been approved by oli-obk

It is now in the queue for this repository.