Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span - #163744
Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span#163744joshtriplett wants to merge 27 commits into
Conversation
…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.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span
This comment has been minimized.
This comment has been minimized.
|
(I can fix up clippy and other things after this gets a perf report.) |
This comment has been minimized.
This comment has been minimized.
|
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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary -0.1%, secondary -1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 491.424s -> 491.272s (-0.03%) |
|
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()`.
|
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()`.
… actually erroring
Avoid doing `path.span().with_hi(path.span().hi())` along some paths.
This provides a non-trivial performance improvement.
Reduce memory usage for Path values with a single Ident, or where we can reconstruct the Span
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary -2.2%, secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 488.486s -> 491.135s (0.54%) |
This comment has been minimized.
This comment has been minimized.
|
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 . |
This comment has been minimized.
This comment has been minimized.
8f87557 to
93bc654
Compare
|
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:
|
This comment has been minimized.
This comment has been minimized.
…_ident` No expected performance impact (just printing code), just a cleanup.
a99858e to
b12956f
Compare
|
Changes to the size of AST and/or HIR nodes. cc @nnethercote The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease
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 If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_attr_ir cc @jdonszelmann, @JonathanBrouwer
cc @rust-lang/clippy Some changes occurred in compiler/rustc_attr_parsing |
|
r? @camelid rustbot has assigned @camelid. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @nnethercote |
|
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. |
There was a problem hiding this comment.
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.
| //! - [`Attribute`]: Metadata associated with item. | ||
| //! - [`UnOp`], [`BinOp`], and [`BinOpKind`]: Unary and binary operators. | ||
|
|
||
| // ignore-tidy-file-filelength |
There was a problem hiding this comment.
Can this be avoided? Feels like a bad idea in a 4,500 line file.
| /// 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. |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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`). |
There was a problem hiding this comment.
Worth mentioning here if/when a single identifier might end up in the Path::General form.
| } | ||
|
|
||
| #[inline] | ||
| pub fn is_empty(&self) -> bool { |
There was a problem hiding this comment.
Are empty paths a thing?
| 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 |
There was a problem hiding this comment.
This line doesn't really fit. Should it remain temporary, local-only code?
| }; | ||
| segments.is_empty() | ||
| match self { | ||
| Path::General((segments, _)) => segments.is_empty(), |
There was a problem hiding this comment.
Shouldn't NoSpan be handled here?
There was a problem hiding this comment.
Oh, I see now NoSpan is always non-empty. Might be worth a brief comment here.
View all comments
Make
Pathan enum, with a variant for just a singleIdent(the common case).The majority of
Pathvalues in the compiler just need a singleIdent, with no arguments, and no separateSpandiffering from the one in theIdent. For instance, local variables, item names, argument names, and so on.However,
Pathstored them as a span and a pointer to a vector (including len/capacity) ofPathSegmentstructs, each containing anIdent, aNodeId, and anOption<Box<GenericArgs>>. And, to make it even worse, these typically have allocated capacity for fourPathSegmentstructs despite only having one. So, in total, a one-IdentPathtook up:SpanandThinVecpointerThinVeclength and capacityPathSegments, 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
Pathinto an enum, with one variantPath::Identfor the single-Ident no-args case, and the other variantPath::Generalfor any case with multiple segments, zero segments, any generic arguments, or aSpanthat doesn't match theIdent.For aws-sdk-ec2 (release-2026-10-02), 55% of all
Pathvalues (806434/1460321) can usePath::Ident.In order to keep
Paththe same size (16 bytes) and not grow all the structures containing it, also avoid storing aSpanforPathvalues 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::NoSpanvariant that just stores the segments.This works for the vast majority of
Pathvalues that otherwise usedPath::General. We can then box the ones that remainPath::General, in order to keepPath16 bytes and avoid growing it or the structures that contain it.For aws-sdk-ec2 (release-2026-10-02), after the 55% of
Pathvalues that can usePath::Ident, another 42% (615812/1460321) can usePath::NoSpan, leaving less than 3% (38075/1460321) that still needPath::General.Add
input-statsmeasurement ofPathdistributions, to collect this data:100% human-written code.