perform less decoding if it has the same syntax context #129827

bvanjoi · 2024-08-31T17:12:47Z

Following this comment

petrochenkov · 2024-08-31T19:20:49Z

perform less decoding if it has the same syntax context Following this [comment](rust-lang#127279 (comment)) r? `@petrochenkov`

bors · 2024-08-31T19:22:02Z

⌛ Trying commit 772fe96 with merge 73a955e...

bors · 2024-08-31T21:14:19Z

☀️ Try build successful - checks-actions
Build commit: 73a955e (73a955eb97a83f4346254767c90b75d80f2196c6)

rust-timer · 2024-08-31T22:30:49Z

Finished benchmarking commit (73a955e): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4%	[0.4%, 0.4%]	2
Regressions ❌ (secondary)	0.6%	[0.5%, 0.7%]	4
Improvements ✅ (primary)	-1.1%	[-2.7%, -0.3%]	3
Improvements ✅ (secondary)	-0.4%	[-0.8%, -0.3%]	15
All ❌✅ (primary)	-0.5%	[-2.7%, 0.4%]	5

Max RSS (memory usage)

Results (primary 0.6%, secondary 0.6%)

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.4%, 2.9%]	104
Regressions ❌ (secondary)	0.8%	[0.4%, 2.6%]	78
Improvements ✅ (primary)	-0.8%	[-1.6%, -0.5%]	14
Improvements ✅ (secondary)	-0.7%	[-1.6%, -0.4%]	18
All ❌✅ (primary)	0.6%	[-1.6%, 2.9%]	118

Cycles

Results (primary -0.1%, secondary 0.2%)

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.3%	[0.4%, 4.4%]	14
Regressions ❌ (secondary)	1.0%	[0.4%, 3.0%]	66
Improvements ✅ (primary)	-0.8%	[-2.7%, -0.4%]	26
Improvements ✅ (secondary)	-0.9%	[-3.8%, -0.4%]	48
All ❌✅ (primary)	-0.1%	[-2.7%, 4.4%]	40

Binary size

Results (primary 0.4%, secondary 0.6%)

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.4%	[0.0%, 1.3%]	47
Regressions ❌ (secondary)	0.6%	[0.3%, 1.1%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.0%, 1.3%]	47

Bootstrap: 789.576s -> 791.168s (0.20%)
Artifact size: 338.70 MiB -> 338.88 MiB (0.05%)

compiler/rustc_span/src/hygiene.rs

compiler/rustc_metadata/src/rmeta/mod.rs

compiler/rustc_middle/src/query/on_disk_cache.rs

bvanjoi · 2024-09-10T17:11:02Z

@rustbot ready

petrochenkov · 2024-09-11T19:50:36Z

@bors try @rust-timer queue

perform less decoding if it has the same syntax context Following this [comment](rust-lang#127279 (comment)) r? `@petrochenkov`

bors · 2024-09-11T19:51:48Z

⌛ Trying commit 6fcc18a with merge 8ec48d7...

bors · 2024-09-11T21:43:17Z

☀️ Try build successful - checks-actions
Build commit: 8ec48d7 (8ec48d717d1582b4447a4253585436174ff15a61)

bvanjoi · 2025-03-29T09:44:36Z

I've made some adjustments based on #139083. Looks like we can remove pending_ctxt now.

petrochenkov · 2025-03-29T12:56:58Z

Thanks!
This looks ready for merging now.
@bors r+

bors · 2025-03-29T12:57:01Z

📌 Commit 366095d has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2025-03-29T16:50:09Z

⌛ Testing commit 366095d with merge d4812c8...

bors · 2025-03-29T19:56:39Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing d4812c8 to master...

github-actions · 2025-03-29T19:58:45Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 5cc6072 (parent) -> d4812c8 (this PR)

Test differences

Show 8 test diffs

Additionally, 8 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Job duration changes

aarch64-gnu-debug: 4169.8s -> 7335.3s (75.9%)
dist-x86_64-apple: 9179.2s -> 10178.9s (10.9%)
dist-apple-various: 7006.5s -> 7673.1s (9.5%)
dist-armhf-linux: 4975.9s -> 5296.0s (6.4%)
i686-mingw-3: 7720.8s -> 8186.3s (6.0%)
i686-msvc-1: 9375.5s -> 9822.0s (4.8%)
dist-riscv64-linux: 4970.0s -> 5188.6s (4.4%)
x86_64-gnu: 6363.7s -> 6589.0s (3.5%)
x86_64-mingw-2: 6812.6s -> 7048.3s (3.5%)
dist-x86_64-freebsd: 4874.3s -> 5033.1s (3.3%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-03-29T21:14:12Z

Finished benchmarking commit (d4812c8): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.1%, 3.8%]	77
Regressions ❌ (secondary)	2.3%	[0.2%, 6.1%]	37
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	0.9%	[0.1%, 3.8%]	77

Max RSS (memory usage)

Results (primary -2.4%, secondary -3.5%)

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)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-3.5%	[-6.6%, -1.7%]	3
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

Results (primary -0.1%, secondary 2.5%)

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.4%, 2.0%]	5
Regressions ❌ (secondary)	2.5%	[1.3%, 4.1%]	3
Improvements ✅ (primary)	-3.0%	[-3.2%, -2.9%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-3.2%, 2.0%]	8

Binary size

Results (primary -0.2%, secondary -0.4%)

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.2%	[-0.9%, -0.0%]	121
Improvements ✅ (secondary)	-0.4%	[-0.9%, -0.1%]	37
All ❌✅ (primary)	-0.2%	[-0.9%, -0.0%]	121

Bootstrap: 777.697s -> 777.207s (-0.06%)
Artifact size: 365.93 MiB -> 365.95 MiB (0.01%)

petrochenkov · 2025-03-29T22:57:42Z

The perf result here is unexpected, this was supposed to be an improvement (and it was an improvement in the previous iteration - #129827 (comment)).
We'll probably have to revert.

This is also why I wanted to do this in smaller steps and not at once (#129827 (comment)).

bvanjoi · 2025-03-30T01:05:32Z

This is quite unexpected. Could we proceed like this:

First, revert that change.
Then merge hygiene: Rewrite apply_mark_internal to be more understandable #139083 (since it doesn't affect performance).
Finally, reimplement the logic to pinpoint the issue.

…rochenkov" Reverting because of a performance regression. This reverts commit d4812c8, reversing changes made to 5cc6072.

Kobzol · 2025-03-30T09:15:41Z

Posted a revert here: #139130.

Revert "Auto merge of rust-lang#129827 - bvanjoi:less-decoding, r=petrochenkov" Reverting rust-lang#129827 because of a performance regression. This reverts commit d4812c8, reversing changes made to 5cc6072. r? `@petrochenkov`

hygiene: Rewrite `apply_mark_internal` to be more understandable The previous implementation allocated new `SyntaxContext`s in the inverted order, and it was generally very hard to understand why its result matches what the `opaque` and `opaque_and_semitransparent` field docs promise. ```rust /// This context, but with all transparent and semi-transparent expansions filtered away. opaque: SyntaxContext, /// This context, but with all transparent expansions filtered away. opaque_and_semitransparent: SyntaxContext, ``` It also couldn't be easily reused for the case where the context id is pre-reserved like in rust-lang#129827. The new implementation tries to follow the docs in a more straightforward way. I did the transformation in small steps, so it indeed matches the old implementation, not just the docs. So I suggest reading only the new version.

panstromek · 2025-03-31T09:51:13Z

Perf triage:

This regression was addressed by the revert.

@rustbot label: +perf-regression-triaged

petrochenkov · 2025-04-01T22:05:01Z

compiler/rustc_span/src/hygiene.rs

-
-        // Reminder: `HygieneDecodeContext` is per-crate, so there are no collisions between
-        // raw ids from different crate metadatas.
-        if let Some(ctxt) = inner.remapped_ctxts.get(raw_id as usize).copied().flatten() {


Removing this cache was probably a bad idea.

I remember that the mechanism here was designed to prevent redundant locking, though I'm not entirely sure if it relates to performance. Let's wait for the performance test results to confirm.

#139228 showed almost the same regressions as #129827 (comment).

@bvanjoi could you resubmit this PR with remapped_ctxts caching restored and #129827 (comment) addressed?

hygiene: Remove all caching in syntax context decoding rust-lang#129827 unintentionally removed one caching layer in syntax context decoding (rust-lang#129827 (comment)), and it was a perf regression. However, it didn't remove all the infrastructure and locks supporting that caching layer. Let's actually try to double down on that change, remove everything and see what happens. If it doesn't work out, we'll try just try to re-land rust-lang#129827 without the `remapped_ctxts` removal. cc `@bvanjoi`

petrochenkov · 2025-04-02T07:09:10Z

compiler/rustc_span/src/hygiene.rs

@@ -1473,29 +1415,10 @@ pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContext
            Some(&ctxt) => ctxt,


This syntax_context_map lookup is no longer necessary, alloc_ctxt/apply_mark_internal already performs it.

petrochenkov · 2025-04-02T07:14:58Z

compiler/rustc_span/src/hygiene.rs

-            return ctxt;
-        }
-
-        match inner.decoding.entry(raw_id) {


The decoding field can also be removed now.

rustbot assigned petrochenkov Aug 31, 2024

rustbot added S-waiting-on-review T-compiler labels Aug 31, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Aug 31, 2024

petrochenkov removed the S-waiting-on-review label Aug 31, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2024

Auto merge of rust-lang#129827 - bvanjoi:less-decoding, r=<try>

73a955e

perform less decoding if it has the same syntax context Following this [comment](rust-lang#127279 (comment)) r? `@petrochenkov`

This comment has been minimized.

Sign in to view

rustbot added perf-regression and removed S-waiting-on-perf labels Aug 31, 2024

petrochenkov added the S-waiting-on-review label Sep 1, 2024

petrochenkov reviewed Sep 5, 2024

View reviewed changes

compiler/rustc_span/src/hygiene.rs Outdated Show resolved Hide resolved

compiler/rustc_metadata/src/rmeta/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/query/on_disk_cache.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author and removed S-waiting-on-review labels Sep 5, 2024

bvanjoi force-pushed the less-decoding branch from 772fe96 to 6fcc18a Compare September 10, 2024 15:57

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Sep 10, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Sep 11, 2024

petrochenkov removed the S-waiting-on-review label Sep 11, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024

Auto merge of rust-lang#129827 - bvanjoi:less-decoding, r=<try>

8ec48d7

perform less decoding if it has the same syntax context Following this [comment](rust-lang#127279 (comment)) r? `@petrochenkov`

This comment has been minimized.

Sign in to view

bvanjoi force-pushed the less-decoding branch from bcdf897 to 9fc9eba Compare March 29, 2025 09:37

less decoding if it has the same syntax context

366095d

bvanjoi force-pushed the less-decoding branch from 9fc9eba to 366095d Compare March 29, 2025 09:44

bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 29, 2025

bors added the merged-by-bors label Mar 29, 2025

bors merged commit d4812c8 into rust-lang:master Mar 29, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Mar 29, 2025

bors mentioned this pull request Mar 29, 2025

hygiene: Rename semi-transparent to semi-opaque #139084

Open

Kobzol added a commit to Kobzol/rust that referenced this pull request Mar 30, 2025

Revert "Auto merge of rust-lang#129827 - bvanjoi:less-decoding, r=pet…

31face9

…rochenkov" Reverting because of a performance regression. This reverts commit d4812c8, reversing changes made to 5cc6072.

Kobzol mentioned this pull request Mar 30, 2025

Revert "Auto merge of #129827 - bvanjoi:less-decoding, r=petrochenkov" #139130

Merged

rustbot added the perf-regression-triaged label Mar 31, 2025

petrochenkov reviewed Apr 1, 2025

View reviewed changes

petrochenkov mentioned this pull request Apr 1, 2025

hygiene: Remove all caching in syntax context decoding #139228

Closed

petrochenkov reviewed Apr 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perform less decoding if it has the same syntax context #129827

perform less decoding if it has the same syntax context #129827

bvanjoi commented Aug 31, 2024

petrochenkov commented Aug 31, 2024

This comment has been minimized.

bors commented Aug 31, 2024

bors commented Aug 31, 2024

This comment has been minimized.

rust-timer commented Aug 31, 2024

bvanjoi commented Sep 10, 2024

petrochenkov commented Sep 11, 2024

This comment has been minimized.

bors commented Sep 11, 2024

bors commented Sep 11, 2024

This comment has been minimized.

bvanjoi commented Mar 29, 2025

petrochenkov commented Mar 29, 2025

bors commented Mar 29, 2025

bors commented Mar 29, 2025

bors commented Mar 29, 2025

github-actions bot commented Mar 29, 2025

rust-timer commented Mar 29, 2025

petrochenkov commented Mar 29, 2025 •

edited

Loading

bvanjoi commented Mar 30, 2025

Kobzol commented Mar 30, 2025

panstromek commented Mar 31, 2025

petrochenkov Apr 1, 2025

bvanjoi Apr 2, 2025

petrochenkov Apr 2, 2025

petrochenkov Apr 2, 2025

petrochenkov Apr 2, 2025

		@@ -1473,29 +1415,10 @@ pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContext
		Some(&ctxt) => ctxt,

perform less decoding if it has the same syntax context #129827

perform less decoding if it has the same syntax context #129827

Conversation

bvanjoi commented Aug 31, 2024

petrochenkov commented Aug 31, 2024

This comment has been minimized.

bors commented Aug 31, 2024

bors commented Aug 31, 2024

This comment has been minimized.

rust-timer commented Aug 31, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

bvanjoi commented Sep 10, 2024

petrochenkov commented Sep 11, 2024

This comment has been minimized.

bors commented Sep 11, 2024

bors commented Sep 11, 2024

This comment has been minimized.

bvanjoi commented Mar 29, 2025

petrochenkov commented Mar 29, 2025

bors commented Mar 29, 2025

bors commented Mar 29, 2025

bors commented Mar 29, 2025

github-actions bot commented Mar 29, 2025

Test differences

Job duration changes

rust-timer commented Mar 29, 2025

Overall result: ❌ regressions - please read the text below

petrochenkov commented Mar 29, 2025 • edited Loading

bvanjoi commented Mar 30, 2025

Kobzol commented Mar 30, 2025

panstromek commented Mar 31, 2025

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

petrochenkov commented Mar 29, 2025 •

edited

Loading