Remove unwraps from metadata decoding: introduce `BlobDecoder` #149455

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

Merged

bors merged 5 commits into rust-lang:main from jdonszelmann:metadata-decoding-s

Dec 8, 2025

Contributor

jdonszelmann commented Nov 29, 2025 •

edited

Loading

Collaborator

rustbot commented Nov 29, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

rustbot added S-waiting-on-review T-compiler labels

jdonszelmann force-pushed the metadata-decoding-s branch from 25b88c6 to cf9344a Compare

November 29, 2025 15:03

This comment has been minimized.

Sign in to view

Contributor Author

jdonszelmann commented Nov 29, 2025

@bors try @rust-timer queue

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request


          Auto merge of #149455 - jdonszelmann:metadata-decoding-s, r=<try>

694a8a4

Metadata decoding s

rustbot added the S-waiting-on-perf label

Contributor Author

jdonszelmann commented Nov 29, 2025

cc: @oli-obk this is what I was talking about. not yet clean, but wanted to see what perf looked like :3

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

rust-bors bot commented Nov 29, 2025

💔 Test for 694a8a4 failed: CI. Failed jobs:

try - dist-x86_64-linux (web logs, extended logs)

jdonszelmann force-pushed the metadata-decoding-s branch from cf9344a to bed17cf Compare

November 29, 2025 20:17

Contributor Author

jdonszelmann commented Nov 29, 2025

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request


          Auto merge of #149455 - jdonszelmann:metadata-decoding-s, r=<try>

7ac6e1b

Metadata decoding s

rust-bors bot commented Nov 29, 2025

☀️ Try build successful (CI)
Build commit: 7ac6e1b (7ac6e1b480efa4bd33f86547b8e73a2122f069d0, parent: 8a3a6bdb68b4d4c9ed922840808b02015741331e)

This comment has been minimized.

Sign in to view

Collaborator

rust-timer commented Nov 29, 2025

Finished benchmarking commit (7ac6e1b): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.2%]	8
Improvements ✅ (secondary)	-0.4%	[-1.4%, -0.1%]	32
All ❌✅ (primary)	-0.2%	[-0.4%, -0.2%]	8

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

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

Bootstrap: 471.041s -> 471.157s (0.02%)
Artifact size: 386.90 MiB -> 386.90 MiB (0.00%)

rustbot removed the S-waiting-on-perf label

Contributor

oli-obk commented Nov 30, 2025

The large workspace benchmark shows improvements in not just instructions, but also time on the write dep info query.

Everything else is a mixed bag on time, unsure why other queries are affected. There are minimal improvements for incremental loading across the board. I'm guessing I'm general the branch predictor just got this all correct.

jdonszelmann force-pushed the metadata-decoding-s branch 2 times, most recently from 524ba79 to 285c9b1 Compare

December 1, 2025 11:26

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                  fn decoder(self, pos: usize) -> Self::Context;

              }

              impl<'a> Metadata<'a> for &'a MetadataBlob {

Contributor Author

jdonszelmann Dec 1, 2025

importantly there are now only two, not 5 implementations of Metadata.

One for MetadataBlob which provides only the BlobDecodeContext which only implements BlobDecoder
One for (tcx, cdata) which provides the MetadataDecodeContext which does implement SpanDecoder and TyDecoder.

This makes it statically known whether we have tcx and cdata available, and whether spans etc can be decoded or not.

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                  fn decode<'a, 'tcx, M: Metadata<'a>>(self, metadata: M) -> T::Value<'tcx>

                  where

                      T::Value<'tcx>: Decodable<DecodeContext<'a, 'tcx>>,

                      T::Value<'tcx>: Decodable<M::Context>,

Contributor Author

jdonszelmann Dec 1, 2025

Decodable specifically with the Context type from whichever kind of M: Metadata is given.

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
              impl<'a, 'tcx> MetadataDecodeContext<'a, 'tcx> {

                  #[inline]

                  fn tcx(&self) -> TyCtxt<'tcx> {

                      let Some(tcx) = self.tcx else {

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      if let Some(alloc_decoding_session) = self.alloc_decoding_session {

                          alloc_decoding_session.decode_alloc_id(self)

                      } else {

                          bug!("Attempting to decode interpret::AllocId without CrateMetadata")

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      let cdata = self.cdata();

                      let Some(sess) = self.sess else {

                          bug!(

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann changed the title ~~Metadata decoding s~~ Remove unwraps from metadata decoding: introduce BlobDecoder

WaffleLapkin requested changes

View reviewed changes

Member

WaffleLapkin left a comment •

edited by rustbot

Loading

doc nits

View changes since this review

compiler/rustc_macros/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_macros/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_macros/src/lib.rs Show resolved Hide resolved

rustbot added S-waiting-on-author and removed S-waiting-on-review labels

Collaborator

rustbot commented Dec 7, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

WaffleLapkin self-assigned this

jdonszelmann added 5 commits

December 8, 2025 00:24


          split out blob decode trait

f8bbf2c


          make cdata required

40a77eb


          remove unnecessary method

395b508


          add TyCtxt everywhere replacing nothing or passed sessions

fa5f075


          document various traits

8c7889b

jdonszelmann force-pushed the metadata-decoding-s branch from ca97283 to 8c7889b Compare

December 7, 2025 23:29

Collaborator

rustbot commented Dec 7, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Member

WaffleLapkin commented Dec 8, 2025

Collaborator

bors commented Dec 8, 2025

📌 Commit 8c7889b has been approved by WaffleLapkin

It is now in the queue for this repository.

bors added S-waiting-on-bors and removed S-waiting-on-author labels

Collaborator

bors commented Dec 8, 2025

⌛ Testing commit 8c7889b with merge 5bc3450...

Collaborator

bors commented Dec 8, 2025

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 5bc3450 to main...

bors added the merged-by-bors label

bors merged commit 5bc3450 into rust-lang:main

12 checks passed

rustbot added this to the 1.94.0 milestone

Contributor

github-actions bot commented Dec 8, 2025

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 03d7ad7 (parent) -> 5bc3450 (this PR)

Test differences

Show 3 test diffs

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

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 5bc345055b06dc096b01bc6d1e0cbe92d3cc630e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

pr-check-1: 1729.2s -> 2048.0s (+18.4%)
aarch64-gnu-llvm-20-2: 2206.2s -> 2579.0s (+16.9%)
dist-x86_64-apple: 8083.0s -> 6830.5s (-15.5%)
x86_64-msvc-ext3: 5758.6s -> 6570.2s (+14.1%)
x86_64-gnu-llvm-21-1: 3115.3s -> 3554.0s (+14.1%)
x86_64-gnu-llvm-20: 2461.1s -> 2770.5s (+12.6%)
aarch64-gnu-llvm-20-1: 3342.6s -> 3758.7s (+12.4%)
x86_64-rust-for-linux: 2826.0s -> 3173.3s (+12.3%)
armhf-gnu: 4919.9s -> 5509.5s (+12.0%)
x86_64-gnu-llvm-20-1: 3185.1s -> 3564.6s (+11.9%)

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.

This was referenced Dec 8, 2025

Externally implementable items #146348

Merged

is_doc_hidden optimized to hook in case of non-incremental build #147511

Open

resolve: Preserve ambiguous glob reexports in crate metadata #147984

Merged

Collaborator

rust-timer commented Dec 8, 2025

Finished benchmarking commit (5bc3450): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.1%]	8
Improvements ✅ (secondary)	-0.4%	[-1.4%, -0.1%]	28
All ❌✅ (primary)	-0.2%	[-0.4%, -0.1%]	8

Max RSS (memory usage)

Results (primary 1.8%, secondary -2.8%)

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

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.6%, 2.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	1.8%	[1.6%, 2.1%]	2

Cycles

Results (primary -2.2%, secondary -1.9%)

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
Improvements ✅ (primary)	-2.2%	[-2.3%, -2.1%]	3
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-2.2%	[-2.3%, -2.1%]	3

Binary size

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

Bootstrap: 472.922s -> 473.701s (0.16%)
Artifact size: 389.01 MiB -> 389.02 MiB (0.00%)

This was referenced Dec 8, 2025

Remove support for prefix yield #148313

Draft

Overhaul filename handling for cross-compiler consistency #149709

Merged

github-actions bot mentioned this pull request

Toolchain upgrade to nightly-2025-12-11 flux-rs/flux#1345

Draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors S-waiting-on-bors T-compiler