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)

rust-timer · 2024-09-11T22:57:29Z

Finished benchmarking commit (8ec48d7): 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
Regressions ❌ (secondary)	0.3%	[0.3%, 0.5%]	4
Improvements ✅ (primary)	-0.4%	[-0.6%, -0.2%]	33
Improvements ✅ (secondary)	-0.3%	[-0.7%, -0.2%]	16
All ❌✅ (primary)	-0.4%	[-0.6%, -0.2%]	33

Max RSS (memory usage)

Results (primary -1.4%, secondary -0.8%)

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)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-1.4%	[-2.7%, -0.8%]	3
Improvements ✅ (secondary)	-4.7%	[-4.7%, -4.7%]	1
All ❌✅ (primary)	-1.4%	[-2.7%, -0.8%]	3

Cycles

Results (primary 6.0%)

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

Binary size

Results (primary -0.5%, secondary -0.7%)

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.5%	[-2.9%, -0.0%]	113
Improvements ✅ (secondary)	-0.7%	[-3.4%, -0.0%]	38
All ❌✅ (primary)	-0.5%	[-2.9%, -0.0%]	113

Bootstrap: 756.954s -> 756.846s (-0.01%)
Artifact size: 341.36 MiB -> 341.33 MiB (-0.01%)

bvanjoi · 2024-09-12T17:10:08Z

compiler/rustc_span/src/hygiene.rs

        {
-            ctxt_data = old.clone();
-        }
+            (cache, hygiene_data.syntax_context_data[cache.0 as usize].clone())


Or maybe this cache is not necessary because we can compute it immediately🤔

petrochenkov · 2024-09-13T15:21:09Z

compiler/rustc_span/src/hygiene.rs


-    // Overwrite the dummy data with our decoded SyntaxContextData
-    HygieneData::with(|hygiene_data| {
-        if let Some(old) = hygiene_data.syntax_context_data.get(raw_id as usize)


I'm rereading the code around here carefully and have just noticed that #127279 uses raw_id to index into hygiene_data.syntax_context_data.
But that doesn't make any sense!

hygiene_data.syntax_context_data is indexed by decoded SyntaxContexts.
remapped_ctxtss are indexed by raw_id.
I guess the raw and decoded values can coincide if they are from the local crate during incremental compilation, and things didn't change too much between two compilations, but that's pure luck.

Also, the idea behind #127279 was probably incorrect after all.

If two syntax contexts have matching (parent, outer_expn_id, outer_transparency) tuples, they shouldn't just get identical (cloned) SyntaxContextDatas, they should have the same SyntaxContext ids.

That's because all hygiene logic runs on comparing SyntaxContext, not SyntaxContextDatas, so two different SyntaxContexts are considered different even if they datas are the same.
In other words, SyntaxContextData are assumed to be unique and interned (like Symbols), and decoding should maintain this property.

That's because all hygiene logic runs on comparing SyntaxContext, not SyntaxContextDatas, so two different SyntaxContexts are considered different even if they datas are the same.

I think this is exactly what #127279 aims to do: reuse the opaque value to ensure it compares the correct SyntaxContext value.

If two syntax contexts have matching (parent, outer_expn_id, outer_transparency) tuples, they shouldn't just get identical (cloned) SyntaxContextDatas, they should have the same SyntaxContext ids.

I'm confused because the old code decodes opaque and generates a new SyntaxContext value using new_ctxt. Following this view, this logic has always been flawed.

Following this view, this logic has always been flawed.

Yep, that seems true.
I'm still investigating.

petrochenkov · 2024-09-13T20:53:02Z

I've fixed the syntax context uniqueness issue in #130324, but using only keys for encoding/decoding, like in this PR, still makes sense.
@rustbot blocked

hygiene: Ensure uniqueness of `SyntaxContextData`s `SyntaxContextData`s are basically interned with `SyntaxContext`s working as keys, so they are supposed to be unique. However, currently duplicate `SyntaxContextData`s can be created during decoding from metadata or incremental cache. This PR fixes that. cc rust-lang#129827 (comment)

rustbot assigned petrochenkov Aug 31, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2024

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 Aug 31, 2024

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 31, 2024

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2024

petrochenkov reviewed Sep 5, 2024

View reviewed changes

compiler/rustc_span/src/hygiene.rs 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 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 Sep 5, 2024

perform less decoding if it has the same syntax context

6fcc18a

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

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

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 Sep 11, 2024

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2024

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2024

bvanjoi commented Sep 12, 2024

View reviewed changes

petrochenkov reviewed Sep 13, 2024

View reviewed changes

petrochenkov mentioned this pull request Sep 13, 2024

hygiene: Ensure uniqueness of SyntaxContextDatas #130324

Open

rustbot 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 Sep 13, 2024

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.

rust-timer commented Sep 11, 2024

bvanjoi Sep 12, 2024

petrochenkov Sep 13, 2024 •

edited

Loading

petrochenkov Sep 13, 2024

bvanjoi Sep 13, 2024

petrochenkov Sep 13, 2024

petrochenkov commented Sep 13, 2024

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

Are you sure you want to change the base?

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.

rust-timer commented Sep 11, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

petrochenkov Sep 13, 2024 • edited Loading

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 Sep 13, 2024

petrochenkov Sep 13, 2024 •

edited

Loading