Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shrink span encoding further #119367

Merged
merged 1 commit into from
Dec 30, 2023
Merged

Shrink span encoding further #119367

merged 1 commit into from
Dec 30, 2023

Conversation

Mark-Simulacrum
Copy link
Member

Spans are now stored in a more compact form which cuts down on at least 1 byte per span (indirect/direct encoding) and at most 3 bytes per span (indirect/direct encoding, context byte, length byte). As a result, libcore metadata shrinks by 1.5MB.

I'm not a huge fan of the fairly manual encoding/decoding from bits implemented here. Something like Tokio's pack abstraction (https://github.com/tokio-rs/tokio/blob/master/tokio/src/util/bit.rs) might be desirable to cut down on some of the shifting etc. We might also say that this isn't worth doing :)

I took a look at copying the span encoding we use in memory (described here). I think the format there makes a lot more sense for in-memory storage where prioritizing a fixed length (i.e., 4 or 8 bytes) is much more important. In metadata, it's much easier for us to have variable-length values, so there's less of a cliff if we don't quite fit. The bit packing scheme there would need changes to fit the varint scheme since it has a lot of all-1s patterns as the "relative offset" form.

@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Dec 28, 2023
@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 28, 2023
@bors
Copy link
Contributor

bors commented Dec 28, 2023

⌛ Trying commit a723228 with merge f0bcb81...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 28, 2023
@bors
Auto merge of rust-lang#119367 - Mark-Simulacrum:relative-spans, r=<try>
f0bcb81 
Shrink span encoding further

Spans are now stored in a more compact form which cuts down on at least 1 byte per span (indirect/direct encoding) and at most 3 bytes per span (indirect/direct encoding, context byte, length byte). As a result, libcore metadata shrinks by 1.5MB.

I'm not a huge fan of the fairly manual encoding/decoding from bits implemented here. Something like Tokio's pack abstraction (https://github.com/tokio-rs/tokio/blob/master/tokio/src/util/bit.rs) might be desirable to cut down on some of the shifting etc. We might also say that this isn't worth doing :)

I took a look at copying the span encoding we use in memory (described [here](https://github.com/rust-lang/rust/blob/master/compiler/rustc_span/src/span_encoding.rs)). I think the format there makes a lot more sense for in-memory storage where prioritizing a fixed length (i.e., 4 or 8 bytes) is much more important. In metadata, it's much easier for us to have variable-length values, so there's less of a cliff if we don't quite fit. The bit packing scheme there would need changes to fit the varint scheme since it has a lot of all-1s patterns as the "relative offset" form.
@bors
Copy link
Contributor

bors commented Dec 28, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Dec 28, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f0bcb81): comparison URL.

Overall result: ✅ improvements - no 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.

@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
Improvements ✅
(primary)		 -0.3% [-0.7%, -0.2%] 32
Improvements ✅
(secondary)		 -0.5% [-1.0%, -0.2%] 17
All ❌✅ (primary) -0.3% [-0.7%, -0.2%] 32

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

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% [-6.3%, -0.1%] 117
Improvements ✅
(secondary)		 -2.9% [-8.5%, -0.0%] 68
All ❌✅ (primary) -2.4% [-6.3%, -0.1%] 117

Bootstrap: 670.201s -> 673.572s (0.50%)
Artifact size: 312.31 MiB -> 311.97 MiB (-0.11%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 28, 2023
wesleywiser
wesleywiser approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with SpanEncodingMode removed

Nice win!

compiler/rustc_metadata/src/rmeta/decoder.rs Show resolved Hide resolved
@Mark-Simulacrum
Shrink span encoding further
09e619d 
Spans are now stored in a more compact form which cuts down on at least
1 byte per span (indirect/direct encoding) and at most 3 bytes per span
(indirect/direct encoding, context byte, length byte). As a result,
libcore metadata shrinks by 1.5MB.
@Mark-Simulacrum
Copy link
Member Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Dec 30, 2023

📌 Commit 09e619d has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 30, 2023
@bors
Copy link
Contributor

bors commented Dec 30, 2023

⌛ Testing commit 09e619d with merge fe2cfd4...

@bors
Copy link
Contributor

bors commented Dec 30, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing fe2cfd4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2023
@bors bors merged commit fe2cfd4 into rust-lang:master Dec 30, 2023
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Dec 30, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fe2cfd4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Improvements ✅
(primary)		 -0.3% [-0.5%, -0.2%] 27
Improvements ✅
(secondary)		 -0.5% [-0.6%, -0.3%] 11
All ❌✅ (primary) -0.3% [-0.5%, -0.2%] 27

Max RSS (memory usage)

Results

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.7% [0.7%, 0.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.4% [-3.1%, -1.6%] 3
All ❌✅ (primary) - - 0

Cycles

Results

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.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.0% [-0.4%, 0.5%] 2

Binary size

Results

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.5% [-6.3%, -0.1%] 118
Improvements ✅
(secondary)		 -2.8% [-8.5%, -0.0%] 72
All ❌✅ (primary) -2.5% [-6.3%, -0.1%] 118

Bootstrap: 668.991s -> 671.951s (0.44%)
Artifact size: 312.11 MiB -> 311.80 MiB (-0.10%)

@wesleywiser wesleywiser mentioned this pull request Jan 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.77.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Mark-Simulacrum @rustbot @rust-timer @bors @wesleywiser