Fix parallel rustc not being reproducible due to unstable sorts of items

[ywxt]

Currently, A tuple (DefId, SymbolName) is used to determine the order of items in the final binary. However DefId is expected as non-deterministic, which leads to some not reproducible issues under parallel compilation. (See #140425 (comment))

Here, we use Span replacing DefId to make the order deterministic.

Items generated by a macro have a same span. But for codegen tests, items are expected the same order between in binary and source. Unluckily, SymbolName isn't predictive. So we use DefPath to distinguish items generated by a same macro.

At the cost of it, we have to manually sort items generated by macros for codegen tests.

This PR is purposed to fix#140425, but seemly works on #140413 too.

This behavior hasn't added into any test until we have a test suit for the parallel frontend. (See #143953)

Update #113349

r? @oli-obk
cc @matthiaskrgr@Zoxc@SparrowLii

[rustbot]

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

[matthiaskrgr]

@bors try @rust-timer queue

[rust-bors]

⌛ Trying commit 030a674 with merge 2e20b9f…

To cancel the try build, run the command @bors try cancel.

[bjorn3]

I don't think this is a proper fix. Multiple items can have the exact same span. And ignoring all spans is valid too. Try ordering based on the DefPathHash instead of the DefId as @Zoxc was alluding to.

Edit: Never mind. That code is actually intended to keep a stable order based on the source location across rustc versions. Maybe only do the sorting based on span when doing codegen tests would be a good idea? I also think span order comparison could be non-deterministic with parallel rustc once we do parallel macro expansion as that can cause the source map order to be non-deterministic.

[ywxt]

I don't think this is a proper fix. Multiple items can have the exact same span. And ignoring all spans is valid too. Try ordering based on the DefPathHash instead of the DefId as @Zoxc was alluding to.

Edit: Never mind. That code is actually intended to keep a stable order based on the source location across rustc versions. Maybe only do the sorting based on span when doing codegen tests would be a good idea? I also think span order comparison could be non-deterministic with parallel rustc once we do parallel macro expansion as that can cause the source map order to be non-deterministic.

You are right. I agree that using span only for codegen tests is a good idea, but we need a brief way to enable/disable it. An environment variable or flag?

[ywxt]

Also A hash would make a completely different order that is unrelated to the source order. Maybe is it confusing?

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2e20b9f (2e20b9f3cfa74d3a05a9d641c94db30bc451dfbb, parent: d242a8bd5a73f633ba1ec5aacf19acf35a3c747d)

[rust-timer]

Finished benchmarking commit (2e20b9f): comparison URL.

Overall result: ❌ regressions - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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)	2.0%	[0.2%, 61.0%]	42
Regressions ❌ (secondary)	0.3%	[0.2%, 1.2%]	28
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.9%, -0.2%]	4
All ❌✅ (primary)	2.0%	[0.2%, 61.0%]	42

Max RSS (memory usage)

Results (primary 4.7%, secondary 4.0%)

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

	mean	range	count
Regressions ❌ (primary)	4.7%	[3.3%, 6.1%]	2
Regressions ❌ (secondary)	4.0%	[2.3%, 6.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.7%	[3.3%, 6.1%]	2

Cycles

Results (primary 20.0%, secondary 4.9%)

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

	mean	range	count
Regressions ❌ (primary)	20.0%	[2.5%, 60.1%]	4
Regressions ❌ (secondary)	6.1%	[2.5%, 8.7%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	20.0%	[2.5%, 60.1%]	4

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%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 465.115s -> 468.017s (0.62%)
Artifact size: 376.87 MiB -> 376.82 MiB (-0.01%)

[SparrowLii]

I guess that parallel macro expansion should not affect the order of source maps? We may confirm this with @petrochenkov .

In this pr's sorting, def_span is redundant in most cases, and we should consider supplementing def_span only if spans are same.

[cjgillot]

Could we stop sorting altogether? We would rely on the order output by the collector, which should be deterministic. Probably far from source order, but deterministic.

[ywxt]

Could we stop sorting altogether? We would rely on the order output by the collector, which should be deterministic. Probably far from source order, but deterministic.

The key is codegen tests need a predictive and deterministic order.

[cjgillot]

Should we adapt the tests or restrict sorting to codegen tests?

[ywxt]

I prefer to adapt the tests, but it may need more effort and process. A more ideal resolution is to avoid regressions in sorting.

[SparrowLii]

@bors try @rust-timer queue

[rust-bors]

⌛ Trying commit f7fba9a with merge e972d15…

To cancel the try build, run the command @bors try cancel.

[rust-bors]

☀️ Try build successful (CI)
Build commit: e972d15 (e972d15b963bba39b5608d11ff9eb2851d7af217, parent: 552904134b564a74489db50aebe7070fdcce895c)

[rust-timer]

Finished benchmarking commit (e972d15): comparison URL.

Overall result: ❌ regressions - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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)	2.3%	[0.2%, 60.7%]	35
Regressions ❌ (secondary)	0.3%	[0.1%, 1.2%]	30
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[0.2%, 60.7%]	35

Max RSS (memory usage)

Results (primary 4.4%, 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)	4.4%	[2.9%, 5.8%]	2
Regressions ❌ (secondary)	2.5%	[2.4%, 2.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.4%	[2.9%, 5.8%]	2

Cycles

Results (primary 24.9%, secondary 0.5%)

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

	mean	range	count
Regressions ❌ (primary)	24.9%	[3.1%, 58.1%]	3
Regressions ❌ (secondary)	3.7%	[3.4%, 4.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.1%, -2.4%]	2
All ❌✅ (primary)	24.9%	[3.1%, 58.1%]	3

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%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 468.885s -> 468.757s (-0.03%)
Artifact size: 376.79 MiB -> 376.86 MiB (0.02%)

[SparrowLii]

Could we stop sorting altogether? We would rely on the order output by the collector, which should be deterministic. Probably far from source order, but deterministic.

We collect items in parallel (here), so the order is not deterministic.

However, during the collecting, we have already obtained Spans for all items, which we can utilize to avoid regression caused by getting them again when sorting.

[ywxt]

I'm aware of the simpler way to add a flag to enable sorting(by Span and DefPath) only within codegen tests. Also, the items collected are deterministic already and don't need be sorted again.

[ywxt]

Hey, all. I have open a new PR to solve it. See #144722 please.

We add a new internal option to only enable sorting for codegen tests, which doesn't influence the normal path.