Skip to content

Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span - #163744

Open
joshtriplett wants to merge 27 commits into
rust-lang:mainfrom
joshtriplett:path-unsegment
Open

joshtriplett wants to merge 27 commits into
rust-lang:mainfrom
joshtriplett:path-unsegment

Conversation

@joshtriplett

@joshtriplett joshtriplett commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

View all comments

Make Path an enum, with a variant for just a single Ident (the common case).

The majority of Path values in the compiler just need a single Ident, with no arguments, and no separate Span differing from the one in the Ident. For instance, local variables, item names, argument names, and so on.

However, Path stored them as a span and a pointer to a vector (including len/capacity) of PathSegment structs, each containing an Ident, a NodeId, and an Option<Box<GenericArgs>>. And, to make it even worse, these typically have allocated capacity for four PathSegment structs despite only having one. So, in total, a one-Ident Path took up:

  • 16 bytes for a Span and ThinVec pointer
  • 16 bytes for the ThinVec length and capacity
  • 4 PathSegments, each 24 bytes,
    for a total of 128 bytes, not counting any allocator overhead.

On large crates like aws-sdk-ec2, this can be a substantial fraction of the memory usage of the AST. (And the HIR, but this commit doesn't try to deal with that yet.)

Turn Path into an enum, with one variant Path::Ident for the single-Ident no-args case, and the other variant Path::General for any case with multiple segments, zero segments, any generic arguments, or a Span that doesn't match the Ident.

For aws-sdk-ec2 (release-2026-10-02), 55% of all Path values (806434/1460321) can use Path::Ident.

In order to keep Path the same size (16 bytes) and not grow all the structures containing it, also avoid storing a Span for Path values where we can reconstruct it from the segments.

If the span we would have stored matches the span from the first segment to the last, use a Path::NoSpan variant that just stores the segments.

This works for the vast majority of Path values that otherwise used Path::General. We can then box the ones that remain Path::General, in order to keep Path 16 bytes and avoid growing it or the structures that contain it.

For aws-sdk-ec2 (release-2026-10-02), after the 55% of Path values that can use Path::Ident, another 42% (615812/1460321) can use Path::NoSpan, leaving less than 3% (38075/1460321) that still need Path::General.

Add input-stats measurement of Path distributions, to collect this data:

ast-stats - Path Ident: 806434  Other segs 0: 0  1: 41816  2: 47050  3: 130254  4+: 434767  NoSpan: 615812

100% human-written code.

…case)

The majority of Path values in the compiler just need a single Ident,
with no arguments, and no separate Span differing from the one in the
Ident. For instance, local variables, item names, argument names, and so
on.

However, Path stored them as a span and a pointer to a vector (including
len/capacity) of PathSegment structs, each containing an Ident, a
NodeId, and an Option<Box<GenericArgs>>. And, to make it even worse,
these typically have allocated capacity for *four* PathSegment structs
despite only having *one*. So, in total, a one-Ident Path took up:
- 16 bytes for a Span and ThinVec pointer
- 16 bytes for the ThinVec length and capacity
- 4 PathSegments, each 24 bytes,
for a total of 128 bytes, not counting any allocator overhead.

On large crates like aws-sdk-ec2, this can be a substantial fraction of
the memory usage of the AST. (And the HIR, but this commit doesn't try
to deal with that yet.)

Turn Path into an enum, with one variant `Path::Ident` for the
single-Ident no-args case, and the other variant `Path::General` for any
case with multiple segments, zero segments, any generic arguments, or
a span that doesn't match the ident.

For aws-sdk-ec2 (release-2026-10-02), 55% of all Path values
(806434/1460321) can use Path::Ident.

This commit *temporarily* increases the size of Path to 24 bytes; a
subsequent commit will re-shrink it to 16 bytes.
… the segments

If the span we would have stored matches the span from the first segment
to the last, use a `Path::NoSpan` variant that just stores the segments.

This works for the vast majority of Path values that otherwise used
`Path::General`. We can then box the ones that remain `Path::General`,
in order to keep `Path` 16 bytes and avoid growing it or the structures
that contain it.
@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2026
@joshtriplett

Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 4, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Oct 4, 2026
Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span
@rust-log-analyzer

This comment has been minimized.

@joshtriplett

Copy link
Copy Markdown
Member Author

(I can fix up clippy and other things after this gets a perf report.)

@rust-bors

rust-bors Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 9fc4eb4 (9fc4eb4b9dc2f54609bbd11ba261542f1142a8a8)
Base parent: 4ddbc06 (4ddbc06ea09abcda34f80baf31cc9bc2686b0ae6)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9fc4eb4): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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.6% [0.1%, 2.0%] 194
Regressions ❌
(secondary)
0.6% [0.1%, 2.0%] 110
Improvements ✅
(primary)
-0.3% [-0.4%, -0.2%] 4
Improvements ✅
(secondary)
-0.4% [-1.0%, -0.1%] 16
All ❌✅ (primary) 0.5% [-0.4%, 2.0%] 198

Max RSS (memory usage)

Results (primary -1.3%, secondary -3.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)
-1.3% [-2.0%, -1.0%] 5
Improvements ✅
(secondary)
-3.9% [-5.3%, -2.1%] 13
All ❌✅ (primary) -1.3% [-2.0%, -1.0%] 5

Cycles

Results (primary -0.1%, secondary -1.1%)

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

mean range count
Regressions ❌
(primary)
3.1% [2.8%, 3.4%] 2
Regressions ❌
(secondary)
2.3% [2.2%, 2.6%] 3
Improvements ✅
(primary)
-2.3% [-2.9%, -1.5%] 3
Improvements ✅
(secondary)
-2.9% [-4.5%, -2.2%] 6
All ❌✅ (primary) -0.1% [-2.9%, 3.4%] 5

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 491.424s -> 491.272s (-0.03%)
Artifact size: 408.69 MiB -> 408.85 MiB (0.04%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 4, 2026
@joshtriplett

Copy link
Copy Markdown
Member Author

Well, great improvement on Max RSS. Had hoped it would be neutral or better on performance, but it's definitely a tiny hit. Will investigate and see if I can improve it.

…ctually erroring

lower_import_res wanted the Span but only used it to report an error;
pass in the &Path instead and defer calling `.span()`.
@joshtriplett

joshtriplett commented Oct 4, 2026 •

Copy link
Copy Markdown
Member Author

I've already found a number of wins to avoid the perf hits; working on those now.

We don't need to call `self.hi_span()` in this case because we know
it'll be `self.prefix.span()`.
Avoid doing `path.span().with_hi(path.span().hi())` along some paths.
This provides a non-trivial performance improvement.
rust-bors Bot pushed a commit that referenced this pull request Oct 5, 2026
Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span
@rust-log-analyzer

This comment has been minimized.

@rust-bors

rust-bors Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 9aec127 (9aec127f9ced90175df4bf12c79e986fa62a0aa3)
Base parent: 4d13b68 (4d13b68badbf76e5c7831291e8fb324eabae2626)

@rust-timer

This comment has been minimized.

@rustbot rustbot added the T-clippy Relevant to the Clippy team. label Oct 5, 2026
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9aec127): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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.4% [0.1%, 1.1%] 55
Regressions ❌
(secondary)
0.4% [0.1%, 0.7%] 55
Improvements ✅
(primary)
-0.3% [-1.1%, -0.1%] 19
Improvements ✅
(secondary)
-0.5% [-1.0%, -0.2%] 25
All ❌✅ (primary) 0.2% [-1.1%, 1.1%] 74

Max RSS (memory usage)

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

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

mean range count
Regressions ❌
(primary)
0.9% [0.9%, 0.9%] 1
Regressions ❌
(secondary)
3.0% [2.2%, 4.6%] 4
Improvements ✅
(primary)
-1.1% [-1.4%, -0.8%] 4
Improvements ✅
(secondary)
-4.2% [-5.9%, -2.1%] 12
All ❌✅ (primary) -0.7% [-1.4%, 0.9%] 5

Cycles

Results (primary -2.2%, secondary 2.0%)

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

mean range count
Regressions ❌
(primary)
1.7% [1.7%, 1.7%] 1
Regressions ❌
(secondary)
6.6% [3.3%, 16.8%] 7
Improvements ✅
(primary)
-2.7% [-5.1%, -1.5%] 8
Improvements ✅
(secondary)
-2.5% [-4.0%, -2.2%] 7
All ❌✅ (primary) -2.2% [-5.1%, 1.7%] 9

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.486s -> 491.135s (0.54%)
Artifact size: 408.62 MiB -> 409.55 MiB (0.23%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2026
@rust-log-analyzer

This comment has been minimized.

@joshtriplett

joshtriplett commented Oct 5, 2026 •

Copy link
Copy Markdown
Member Author

I've eliminated most of the regressions. rayon-rs/either#146 and rayon-rs/either#147 will help once available, as will mozilla/thin-vec#99 .

@rustbot rustbot added the T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue. label Oct 5, 2026
@rust-log-analyzer

This comment has been minimized.

@joshtriplett

Copy link
Copy Markdown
Member Author

Several additional potential wins worth exploring after this PR, which I'd like to avoid stacking on top because they'd make it larger to review:

  • Add a fast path to parsing that avoids allocating a ThinVec of segments in the common case of a single identifier. This would require unrolling one iteration of parse_path_segments into parse_path_inner, creating a Path::Ident in that case, or continuing on to accumulate the remaining segments if there are more. The primary complication here, and the reason I didn't go on to do it directly in this PR, is that this also needs to handle the recovery cases (or at least detect and punt them to parse_path_segments).
  • Make Path generic, so it also supports a QPath that stores a QSelf directly in Path::General. This would shrink all the structures that keep a QSelf side-by-side with a Path. Any Path associated with a QSelf will use Path::General.
  • Make UsePathList store &Path rather than &[PathSegment], so that creating a UsePathList doesn't require forcing a less ideal Path representation. AFAICT, UsePathList uses that for diagnostics, but currently always has to obtain it in order to reference it, even in builds with no diagnostics.
  • Optimizing HIR paths. These don't have excess capacity (unlike AST paths), because they use an arena slice rather than a ThinVec, but they could still store the simple ident case much more efficiently.

@rust-log-analyzer

This comment has been minimized.

…_ident`

No expected performance impact (just printing code), just a cleanup.
@joshtriplett
joshtriplett marked this pull request as ready for review October 6, 2026 01:09
@rustbot

rustbot commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

rustfmt is developed in its own repository. If possible, consider making this change to rust-lang/rustfmt instead.

cc @rust-lang/rustfmt

Some changes occurred to diagnostic attributes.

cc @mejrs

Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs

cc @ZuseZ4

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann, @JonathanBrouwer

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

@rustbot 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 Oct 6, 2026
@rustbot

rustbot commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

r? @camelid

rustbot has assigned @camelid.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 77 candidates
  • Random selection from 18 candidates

@joshtriplett

Copy link
Copy Markdown
Member Author

r? @nnethercote

@rustbot rustbot assigned nnethercote and unassigned camelid Oct 6, 2026
@joshtriplett

Copy link
Copy Markdown
Member Author

I'd recommend reviewing commit by commit. The first three commits are the bulk of the semantic change, which converts callers very mechanically to make sure the conversion is always obvious. The rest is optimization to avoid performance regression, and the optimizations tend to be pretty self-contained and obvious.

@nnethercote nnethercote left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code changes seem fine; very tedious but quite mechanical. A couple of nits below.

My main concern is the perf results don't yet seem to justify the effort. Instruction counts get a bit worse, memory usage improves a little but not that much. If any of the pending future improvements are likely to make a big difference, it would be good to see them here as well.

View changes since this review

//! - [`Attribute`]: Metadata associated with item.
//! - [`UnOp`], [`BinOp`], and [`BinOpKind`]: Unary and binary operators.

// ignore-tidy-file-filelength

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be avoided? Feels like a bad idea in a 4,500 line file.

Comment thread compiler/rustc_ast/src/ast.rs Outdated
/// The common case of a single identifier (e.g. `x`)
Ident { ident: Ident, id: NodeId },
General {
/// The span of the whole path; might differ from the combined spans of the segments.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you briefly explain how it might differ?

segment.ident.stable_hash(hcx, hasher);
}
self.num_segments().stable_hash(hcx, hasher);
self.iter_idents().for_each(|ident| ident.stable_hash(hcx, hasher));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but it seems weird/wrong that args isn't hashed!

/// E.g., `std::cmp::PartialEq`.
/// We separate the common case a single identifier (e.g. `x`) from the general case of a sequence
/// of identifiers that might also have generics attached (e.g. `std::cmp::PartialEq`,
/// `Vec::<T>::new`).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth mentioning here if/when a single identifier might end up in the Path::General form.

}

#[inline]
pub fn is_empty(&self) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are empty paths a thing?

Comment thread tests/ui/stats/input-stats.stderr Outdated
ast-stats ----------------------------------------------------------------
ast-stats Total 6_624 109
ast-stats ----------------------------------------------------------------
ast-stats - Path Ident: 17 General segs 0: 0 1: 1 2: 2 3: 3 4+: 1 Span reconstructible: 7

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line doesn't really fit. Should it remain temporary, local-only code?

};
segments.is_empty()
match self {
Path::General((segments, _)) => segments.is_empty(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't NoSpan be handled here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I see now NoSpan is always non-empty. Might be worth a brief comment here.

This branch has not been deployed

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

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants