[PERF] Sized Hierarchy: Attempt 1 #141015
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce the `MetaSized` and `PointeeSized` traits as supertraits of `Sized` and initially implement it on everything that currently implements `Sized` to isolate any changes that simply adding the traits introduces.
Expand the automatic implementation of `MetaSized` and `PointeeSized` so that it is also implemented on non-`Sized` types, just not `ty::Foreign` (extern type).
Add `MetaSized` and `PointeeSized` to minicore so that fewer tests fail from missing language items.
As before, add `MetaSized` and `PointeeSized` traits to all of the non-minicore `no_core` tests so that they don't fail for lack of language items.
As core uses an extern type (`ptr::VTable`), the default `?Sized` to `MetaSized` migration isn't sufficient, and some code that previously accepted `VTable` needs relaxed to continue to accept extern types. Similarly, the compiler uses many extern types in `rustc_codegen_llvm` and in the `rustc_middle::ty::List` implementation (`OpaqueListContents`) some bounds must be relaxed to continue to accept these types. Unfortunately, due to the current inability to relax `Deref::Target`, some of the bounds in the standard library are forced to be stricter than they ideally would be.
Adding a sizedness supertrait shouldn't require multiple vtables so shouldn't be linted against.
Opting-out of `Sized` with `?Sized` is now equivalent to adding a `MetaSized` bound, and adding a `MetaSized` or `PointeeSized` bound is equivalent to removing the default `Sized` bound - this commit implements this change in `rustc_hir_analysis::hir_ty_lowering`. `MetaSized` is also added as a supertrait of all traits, as this is necessary to preserve backwards compatibility. Unfortunately, non-global where clauses being preferred over item bounds (where `PointeeSized` bounds would be proven) - which can result in errors when a `PointeeSized` supertrait/bound/predicate is added to some items. Rather than `PointeeSized` being a bound on everything, it can be the absence of a bound on everything, as `?Sized` was.
With `MetaSized` bounds replacing `?Sized` and being added as a supertrait, the same relaxations applied to the standard library must be applied to minicore.
`Sized` errors are currently stashed to improve diagnostics and this
must happen with `{Meta,Pointee}Sized` too to maintain diagnostic
output.
Given the necessary additions of bounds to these traits and their impls in the standard library, it is necessary to add `MetaSized` bounds to the obligation which is proven as part of checking for dyn dispatchability.
Like `Sized` diagnostics, sorting `MetaSized` and `PointeeSized` diagnostics last prevents earlier more useful diagnostics from being skipped because there has already been error tainting.
When printing impl headers in a diagnostic, the compiler has to account for `?Sized` implying `MetaSized` and new `MetaSized` and `PointeeSized` bounds.
When `sized_hierarchy` is enabled, rustc should print `MetaSized` or `PointeeSized` instead of `?Sized` in opaques.
These tests necessarily need to change now that `?Sized` is not sufficient to accept extern types and `PointeeSized` is now necessary. In addition, the `size_of_val`/`align_of_val` test can now be changed to expect an error.
With the addition of new bounds to the unconstrained parameters, there are more errors which just need blessed.
It seems like generics from `non_lifetime_binders` don't have any default bounds like normal generics, so all of the `?Sized` relaxations need to be further relaxed with `PointeeSized` for this test to be the equivalent of before.
This test no longer crashes the compiler as `Box` no longer accepts `PointeeSized`-types. It eventually could, but not because of `Deref::Target` currently, so this doesn't fail anymore and there wasn't an obvious to add new types to make it continue to fail because `Deref` is special.
This test case is a reduction from the `hwc` crate on GitHub, following a crater run. It passes with the next solver but fails on the current solver due to a known limitation of the current solver. It starts fails on the current solver with the `sized_hierarchy` changes because `?Sized` is now a proper bound.
These tests just need blessing, they don't have any interesting behaviour changes. Some of these tests have new errors because `LegacyReceiver` cannot be proven to be implemented now that it is also testing for `MetaSized` - but this is just a consequence of the other errors in the test.
Extend the fast path for `Sized` traits to include constness and `MetaSized`.
As a performance optimization, skip elaborating the supertraits of `Sized`, and if a `MetaSized` obligation is being checked, then look for a `Sized` predicate in the parameter environment. This makes the `ParamEnv` smaller which should improve compiler performance as it avoids all the iteration over the larger `ParamEnv`.
`nominal_obligations` calls `predicates_of` on a `Sized` obligation, effectively elaborating the trait and making the well-formedness checking machinery do a bunch of extra work checking a `MetaSized` obligation is well-formed, but given that both `Sized` and `MetaSized` are built-ins, if `Sized` is otherwise well-formed, so `MetaSized` will be.
These should never be shown to users at the moment.
Some rustdoc tests are `no_core` and need to have `MetaSized` and `PointeeSized` added to them.
As before, updating types using extern types to use `PointeeSized` bounds.
Unexpected Clippy lint triggering is fixed in upcoming commits but is necessary for `cfg(bootstrap)`.
Existing lints that had special-casing for `Sized` predicates ought to have these same special cases applied to `MetaSized` predicates.
One clippy test is `no_core` and needs to have `MetaSized` and `PointeeSized` added to it.
As in many previous commits, adding the new traits to minicore, but this time for cranelift and gcc.
rustbot
added
A-run-make
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 14, 2025
…<try> [PERF] Sized Hierarchy: Attempt 1 A perf run reverting changes to the unelaborated sizedness optimisation on rust-lang#137944 to determine if it caused the regression post-review feedback. r? `@ghost`
|
⌛ Trying commit a7b89ef with merge 41554dfba1e031aed63deed65a2ff1a951cf5874... |
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
the
S-waiting-on-author
|
a network issue, it seems @bors try |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 14, 2025
…<try> [PERF] Sized Hierarchy: Attempt 1 A perf run reverting changes to the unelaborated sizedness optimisation on rust-lang#137944 to determine if it caused the regression post-review feedback. r? `@ghost`
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f6601df): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking 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 @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.9%, secondary 0.3%)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.
CyclesResults (primary 2.0%, secondary -4.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.
Binary sizeResults (primary 0.0%, secondary 0.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.
Bootstrap: 773.561s -> 777.122s (0.46%) |
rustbot
added
perf-regression
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A perf run reverting changes to the unelaborated sizedness optimisation on #137944 to determine if it caused the regression post-review feedback.
r? @ghost