Sort MonoItems by span instead of DefIndex. #92323

cjgillot · 2021-12-27T17:05:30Z

The codegen tests rely on items being process in the same order as they appear in the file.

Instead of sorting them by DefIndex, we should just sort them by def_span.

cc #90317.

rust-highfive · 2021-12-27T17:05:33Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

pierwill · 2022-01-12T18:27:19Z

Thanks for fixing this, @cjgillot! :)

tmiasko · 2022-01-16T09:51:31Z

@bors try @rust-timer queue

rust-timer · 2022-01-16T09:51:32Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-16T09:51:40Z

⌛ Trying commit ae6ce468dd80df306640a56eebc4d170b804b259 with merge ad15d5d984b71fc84a26a3c31579c7cc4b0e1ac9...

bors · 2022-01-16T11:06:40Z

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

rust-timer · 2022-01-16T11:06:42Z

Queued ad15d5d984b71fc84a26a3c31579c7cc4b0e1ac9 with parent 26c06cf, future comparison URL.

rust-timer · 2022-01-16T12:37:12Z

Finished benchmarking commit (ad15d5d984b71fc84a26a3c31579c7cc4b0e1ac9): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -2.4% on incr-unchanged builds of deeply-nested-async)
Large regression in instruction counts (up to 3.7% on full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led 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-review -S-waiting-on-perf +perf-regression

tmiasko · 2022-01-16T12:51:48Z

compiler/rustc_middle/src/mir/mono.rs

+                        // the codegen tests and can even make item order
+                        // unstable.
+                        InstanceDef::Item(def) => {
+                            def.did.as_local().map(|def_id| tcx.def_span(def_id))


Maybe extract a single BytePos? It would reduce the number of conversions from Span to SpanData, which could be perf significant with incremental relative spans.

OK, so it looks like we also need to include SyntaxContext to obtain "natural" order.

In that case, one thing I don't understand, what allows us to rely onSyntaxContext order? Doesn't it suffer from the similar issue as DefIndex given that we remap IDs when loading them from on disk cache, and then sort based on their values?

Marking as blocked until we reach a strategy for SyntaxContext. Each additional use of its PartialOrd impl will make it even harder to remove.

cjgillot · 2022-01-16T15:05:12Z

@bors try @rust-timer queue

rust-timer · 2022-01-16T15:05:13Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-16T15:05:21Z

⌛ Trying commit 08c8028afda89f06cd4fae1ae7469a10db005094 with merge 1b73506c363c317e60a0d8c2196dd03b8372cdec...

bors · 2022-01-16T16:23:49Z

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

rust-timer · 2022-01-16T16:23:50Z

Queued 1b73506c363c317e60a0d8c2196dd03b8372cdec with parent 7be8693, future comparison URL.

rust-timer · 2022-01-16T20:14:58Z

Finished benchmarking commit (1b73506c363c317e60a0d8c2196dd03b8372cdec): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -2.4% on incr-unchanged builds of deeply-nested-async)
Large regression in instruction counts (up to 3.7% on full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led 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-review -S-waiting-on-perf +perf-regression

bors · 2022-04-13T14:01:24Z

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

cjgillot · 2022-08-06T21:35:14Z

@bors try @rust-timer queue

rust-timer · 2022-08-06T21:35:16Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-08-06T21:35:25Z

⌛ Trying commit e64ba94 with merge 915673e44f6d86547c2ba2e330e57a5d0060b87f...

bors · 2022-08-06T23:25:47Z

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

rust-timer · 2022-08-06T23:25:49Z

Queued 915673e44f6d86547c2ba2e330e57a5d0060b87f with parent 44bd81d, future comparison URL.

rust-timer · 2022-08-07T01:44:03Z

Finished benchmarking commit (915673e44f6d86547c2ba2e330e57a5d0060b87f): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.3%	-1.4%	6
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.4%	2.4%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.9%	2.9%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.9%	2.9%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rust-highfive assigned estebank Dec 27, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 27, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2021

This comment has been minimized.

Sign in to view

cjgillot force-pushed the sort-span branch from 726de8e to fd93f55 Compare December 27, 2021 17:17

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 Jan 16, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 16, 2022

tmiasko approved these changes Jan 16, 2022

View reviewed changes

cjgillot force-pushed the sort-span branch 2 times, most recently from ef3e932 to 08c8028 Compare January 16, 2022 14:21

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 16, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 16, 2022

cjgillot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2022

cjgillot force-pushed the sort-span branch from 08c8028 to 35b44cb Compare March 5, 2022 09:07

cjgillot added 3 commits August 6, 2022 19:47

Sort MonoItems by span instead of definition index.

a62a464

Directly cache the SpanData.

6b725d2

Adjust assembly tests.

e64ba94

cjgillot force-pushed the sort-span branch from 35b44cb to e64ba94 Compare August 6, 2022 18:10

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

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Aug 7, 2022

cjgillot closed this Aug 7, 2022

cjgillot deleted the sort-span branch August 7, 2022 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort MonoItems by span instead of DefIndex. #92323

Sort MonoItems by span instead of DefIndex. #92323

cjgillot commented Dec 27, 2021

rust-highfive commented Dec 27, 2021

This comment has been minimized.

This comment has been minimized.

pierwill commented Jan 12, 2022

tmiasko commented Jan 16, 2022

rust-timer commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

rust-timer commented Jan 16, 2022

rust-timer commented Jan 16, 2022

tmiasko Jan 16, 2022

tmiasko Jan 16, 2022

cjgillot Feb 2, 2022

cjgillot commented Jan 16, 2022

rust-timer commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

rust-timer commented Jan 16, 2022

rust-timer commented Jan 16, 2022

bors commented Apr 13, 2022

cjgillot commented Aug 6, 2022

rust-timer commented Aug 6, 2022

bors commented Aug 6, 2022

bors commented Aug 6, 2022

rust-timer commented Aug 6, 2022

rust-timer commented Aug 7, 2022

Sort MonoItems by span instead of DefIndex. #92323

Sort MonoItems by span instead of DefIndex. #92323

Conversation

cjgillot commented Dec 27, 2021

rust-highfive commented Dec 27, 2021

This comment has been minimized.

This comment has been minimized.

pierwill commented Jan 12, 2022

tmiasko commented Jan 16, 2022

rust-timer commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

rust-timer commented Jan 16, 2022

rust-timer commented Jan 16, 2022

tmiasko Jan 16, 2022

Choose a reason for hiding this comment

tmiasko Jan 16, 2022

Choose a reason for hiding this comment

cjgillot Feb 2, 2022

Choose a reason for hiding this comment

cjgillot commented Jan 16, 2022

rust-timer commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

rust-timer commented Jan 16, 2022

rust-timer commented Jan 16, 2022

bors commented Apr 13, 2022

cjgillot commented Aug 6, 2022

rust-timer commented Aug 6, 2022

bors commented Aug 6, 2022

bors commented Aug 6, 2022

rust-timer commented Aug 6, 2022

rust-timer commented Aug 7, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes