Clarify Layout interning.
#94690
Clarify Layout interning.
#94690
Conversation
`Layout` is another type that is sometimes interned, sometimes not, and we always use references to refer to it so we can't take any advantage of the uniqueness properties for hashing or equality checks. This commit renames `Layout` as `LayoutS`, and then introduces a new `Layout` that is a newtype around an `Interned<LayoutS>`. It also interns more layouts than before. Previously layouts within layouts (via the `variants` field) were never interned, but now they are. Hence the lifetime on the new `Layout` type. Unlike other interned types, these ones are in `rustc_target` instead of `rustc_middle`. This reflects the existing structure of the code, which does layout-specific stuff in `rustc_target` while `TyAndLayout` is generic over the `Ty`, allowing the type-specific stuff to occur in `rustc_middle`. The commit also adds a `HashStable` impl for `Interned`, which was needed. It hashes the contents, unlike the `Hash` impl which hashes the pointer.
rustbot
added
T-compiler
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 4f008e0 with merge 5e62d25397a820b5ad1233e0003f4c7d152ba8d9... |
|
☀️ Try build successful - checks-actions |
|
Queued 5e62d25397a820b5ad1233e0003f4c7d152ba8d9 with parent 8876ca3, future comparison URL. |
|
Finished benchmarking commit (5e62d25397a820b5ad1233e0003f4c7d152ba8d9): comparison url. Summary: This benchmark run did not return any relevant results. 9 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. 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 led to changes in compiler perf. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
No notable perf effects, much as expected. @bors rollup=iffy |
| @@ -1377,7 +1391,10 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { | |||
|
|
|||
| let largest_niche = Niche::from_scalar(dl, Size::ZERO, tag); | |||
|
|
|||
| let tagged_layout = Layout { | |||
| let layout_variants = | |||
| layout_variants.into_iter().map(|v| tcx.intern_layout(v)).collect(); | |||
There was a problem hiding this comment.
I wonder if we could intern them beforehand to avoid another loop? the perf didn't show any changes so I don't know if it matters.
|
LGTM. Thanks! @bors r+ |
|
📌 Commit 4f008e0 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Thank you for the fast review. |
|
Finished benchmarking commit (ecb867e): comparison url. Summary: This benchmark run did not return any relevant results. 14 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
… r=fee1-dead Clarify `Layout` interning. `Layout` is another type that is sometimes interned, sometimes not, and we always use references to refer to it so we can't take any advantage of the uniqueness properties for hashing or equality checks. This commit renames `Layout` as `LayoutS`, and then introduces a new `Layout` that is a newtype around an `Interned<LayoutS>`. It also interns more layouts than before. Previously layouts within layouts (via the `variants` field) were never interned, but now they are. Hence the lifetime on the new `Layout` type. Unlike other interned types, these ones are in `rustc_target` instead of `rustc_middle`. This reflects the existing structure of the code, which does layout-specific stuff in `rustc_target` while `TyAndLayout` is generic over the `Ty`, allowing the type-specific stuff to occur in `rustc_middle`. The commit also adds a `HashStable` impl for `Interned`, which was needed. It hashes the contents, unlike the `Hash` impl which hashes the pointer. r? `@fee1-dead`
Layoutis another type that is sometimes interned, sometimes not, andwe always use references to refer to it so we can't take any advantage
of the uniqueness properties for hashing or equality checks.
This commit renames
LayoutasLayoutS, and then introduces a newLayoutthat is a newtype around anInterned<LayoutS>. It alsointerns more layouts than before. Previously layouts within layouts
(via the
variantsfield) were never interned, but now they are. Hencethe lifetime on the new
Layouttype.Unlike other interned types, these ones are in
rustc_targetinstead ofrustc_middle. This reflects the existing structure of the code, whichdoes layout-specific stuff in
rustc_targetwhileTyAndLayoutisgeneric over the
Ty, allowing the type-specific stuff to occur inrustc_middle.The commit also adds a
HashStableimpl forInterned, which wasneeded. It hashes the contents, unlike the
Hashimpl which hashes thepointer.
r? @fee1-dead