update `hash_stable` for `List<Ty<'tcx>>` #94799

lcnr · 2022-03-10T07:31:12Z

cc #93505 (comment)

this is the hottest part changed since the pre-merge perf run

rust-highfive · 2022-03-10T07:31:15Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

lcnr · 2022-03-10T07:31:22Z

@bors try @rust-timer queue

rust-timer · 2022-03-10T07:31:23Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-03-10T07:31:35Z

⌛ Trying commit 299dd02 with merge f93e80bc1db68d23901d970c4aec9e66a47c4b27...

bors · 2022-03-10T08:49:27Z

☀️ Try build successful - checks-actions
Build commit: f93e80bc1db68d23901d970c4aec9e66a47c4b27 (f93e80bc1db68d23901d970c4aec9e66a47c4b27)

rust-timer · 2022-03-10T08:49:28Z

Queued f93e80bc1db68d23901d970c4aec9e66a47c4b27 with parent 01ad0ad, future comparison URL.

compiler/rustc_middle/src/ty/impls_ty.rs

rust-timer · 2022-03-10T11:34:41Z

Finished benchmarking commit (f93e80bc1db68d23901d970c4aec9e66a47c4b27): comparison url.

Summary: This benchmark run did not return any relevant results.

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 label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

lcnr · 2022-03-10T11:45:40Z

will probably have to try something different

@bors try @rust-timer queue

rust-timer · 2022-03-10T11:45:41Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-03-10T11:45:48Z

⌛ Trying commit 5b6b348 with merge b9782aa0f530788d0f64887480f97c9f895815d2...

bors · 2022-03-10T12:56:09Z

☀️ Try build successful - checks-actions
Build commit: b9782aa0f530788d0f64887480f97c9f895815d2 (b9782aa0f530788d0f64887480f97c9f895815d2)

rust-timer · 2022-03-10T12:56:11Z

Queued b9782aa0f530788d0f64887480f97c9f895815d2 with parent d7b282b, future comparison URL.

rust-timer · 2022-03-10T16:58:25Z

Finished benchmarking commit (b9782aa0f530788d0f64887480f97c9f895815d2): comparison url.

Summary: This benchmark run did not return any relevant results.

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 label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

lcnr · 2022-03-10T17:22:15Z

alright, wasn't the reason for the perf impact, but I still believe that this is a small improvement. If this is merged nobody else will try the same change in the future and it doesn't make anything worse 🤷

@bors ready

petrochenkov · 2022-03-10T17:57:26Z

I don't understand what was the point of #93505.
It was supposed to be an optimization (at cost of some extra complexity), but it actually wasn't. Shouldn't it be reverted entirely?

petrochenkov · 2022-03-10T18:02:12Z

I also don't understand how the HashStable works and why it's correct.
In case of enums like

enum E {
  A(u8),
  B(u8),
}

hashing the discriminant (or some other distinct number) is necessary because the payloads have the same type and may collide.

But with

enum E {
  A(OneType),
  B(EntirelyDifferentType),
}

it's not clear why the discriminant should be hashed, and why hashing OneType + discriminant and EntirelyDifferentType + discriminant has any lower chance of collision than OneType and EntirelyDifferentType.

lcnr · 2022-03-11T08:49:36Z

I don't understand what was the point of #93505. It was supposed to be an optimization (at cost of some extra complexity), but it actually wasn't. Shouldn't it be reverted entirely?

I mostly added this to enable ty::Tuple to use List<Ty<'tcx>> instead of List<GenericArg<'tcx>>. Going to try to keep the List<Ty<'tcx>> and List<GenericArg<'tcx>> split in a separate PR to check the impact of that.

I also don't understand how the HashStable works and why it's correct. In case of enums like
enum E {
  A(u8),
  B(u8),
}
hashing the discriminant (or some other distinct number) is necessary because the payloads have the same type and may collide.

But with
enum E {
  A(OneType),
  B(EntirelyDifferentType),
}
it's not clear why the discriminant should be hashed, and why hashing OneType + discriminant and EntirelyDifferentType + discriminant has any lower chance of collision than OneType and EntirelyDifferentType.

if you have

enum OneType {
    VariantA(u32),
    VariantB(u8),
}

enum EntirelyDifferentType {
    VariantA,
    VariantB(u8),
}

hashing E without considering its discriminant would result in E::A(OneType::VariantB(3)) and E::B(EntirelyDifferentType::VariantB(3)) to have the same hash.

I don't think it is trivially apparent that ty::RegionKind and ty::ConstKind do not have a same-ish variants with the same discriminant.

petrochenkov · 2022-03-11T10:21:58Z

I'll mark this as blocked on #94841 for now.
@rustbot blocked

lcnr · 2022-05-09T16:47:05Z

keeping the reasoning from #94799 (comment)

alright, wasn't the reason for the perf impact, but I still believe that this is a small improvement. If this is merged nobody else will try the same change in the future and it doesn't make anything worse 🤷

@petrochenkov depending on your opinion here, feel free to either approve or close this PR
@rustbot ready

petrochenkov · 2022-05-10T10:09:12Z

@bors r+

bors · 2022-05-10T10:09:13Z

📌 Commit 5b6b348 has been approved by petrochenkov

bors · 2022-05-10T10:53:50Z

⌛ Testing commit 5b6b348 with merge c51871c...

bors · 2022-05-10T13:22:55Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing c51871c to master...

rust-timer · 2022-05-10T14:50:41Z

Finished benchmarking commit (c51871c): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

…errors Don't transmute `&List<GenericArg>` <-> `&List<Ty>` In rust-lang#93505 we allowed safely transmuting between `&List<GenericArg<'_>>` and `&List<Ty<'_>>`. This was possible because `GenericArg` is a tagged pointer and the tag for types is `0b00`, such that a `GenericArg` with a type inside has the same layout as `Ty`. While this was meant as an optimization, it doesn't look like it was actually any perf or max-rss win (see rust-lang#94799 (comment), rust-lang#94841, rust-lang#110496 (comment)). Additionally the way it was done is quite fragile — `unsafe` code was not properly documented or contained in a module, types were not marked as `repr(C)` (making the transmutes possibly unsound). All of this makes the code maintenance harder and blocks other possible optimizations (as an example I've found out about these `transmutes` when my change caused them to sigsegv compiler). Thus, I think we can safely (pun intended) remove those transmutes, making maintenance easier, optimizations possible, code less cursed, etc. r? `@compiler-errors`

Don't transmute `&List<GenericArg>` <-> `&List<Ty>` In #93505 we allowed safely transmuting between `&List<GenericArg<'_>>` and `&List<Ty<'_>>`. This was possible because `GenericArg` is a tagged pointer and the tag for types is `0b00`, such that a `GenericArg` with a type inside has the same layout as `Ty`. While this was meant as an optimization, it doesn't look like it was actually any perf or max-rss win (see rust-lang/rust#94799 (comment), rust-lang/rust#94841, rust-lang/rust#110496 (comment)). Additionally the way it was done is quite fragile — `unsafe` code was not properly documented or contained in a module, types were not marked as `repr(C)` (making the transmutes possibly unsound). All of this makes the code maintenance harder and blocks other possible optimizations (as an example I've found out about these `transmutes` when my change caused them to sigsegv compiler). Thus, I think we can safely (pun intended) remove those transmutes, making maintenance easier, optimizations possible, code less cursed, etc. r? `@compiler-errors`

update hash_stable for List<Ty<'tcx>>

299dd02

rust-highfive assigned petrochenkov Mar 10, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 10, 2022

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022

Kobzol reviewed Mar 10, 2022

View reviewed changes

compiler/rustc_middle/src/ty/impls_ty.rs Outdated Show resolved Hide resolved

don't hash ()

5b6b348

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 10, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 10, 2022

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 10, 2022

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2022

lcnr mentioned this pull request Mar 11, 2022

no more transmute<&List<Ty<'tcx>>, &List<GenericArg<'tcx>>> #94841

Closed

petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 9, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 10, 2022

bors merged commit c51871c into rust-lang:master May 10, 2022

rustbot added this to the 1.62.0 milestone May 10, 2022

lcnr deleted the list-ty-perf branch May 10, 2022 16:21

compiler-errors mentioned this pull request Apr 18, 2023

Don't transmute &List<GenericArg> <-> &List<Ty> #110496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update `hash_stable` for `List<Ty<'tcx>>` #94799

update `hash_stable` for `List<Ty<'tcx>>` #94799

lcnr commented Mar 10, 2022

rust-highfive commented Mar 10, 2022

lcnr commented Mar 10, 2022

rust-timer commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

rust-timer commented Mar 10, 2022

rust-timer commented Mar 10, 2022

lcnr commented Mar 10, 2022

rust-timer commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

rust-timer commented Mar 10, 2022

rust-timer commented Mar 10, 2022

lcnr commented Mar 10, 2022 •

edited

Loading

petrochenkov commented Mar 10, 2022

petrochenkov commented Mar 10, 2022

lcnr commented Mar 11, 2022

petrochenkov commented Mar 11, 2022

lcnr commented May 9, 2022 •

edited

Loading

petrochenkov commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

rust-timer commented May 10, 2022

update hash_stable for List<Ty<'tcx>> #94799

update hash_stable for List<Ty<'tcx>> #94799

Conversation

lcnr commented Mar 10, 2022

rust-highfive commented Mar 10, 2022

lcnr commented Mar 10, 2022

rust-timer commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

rust-timer commented Mar 10, 2022

rust-timer commented Mar 10, 2022

lcnr commented Mar 10, 2022

rust-timer commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

rust-timer commented Mar 10, 2022

rust-timer commented Mar 10, 2022

lcnr commented Mar 10, 2022 • edited Loading

petrochenkov commented Mar 10, 2022

petrochenkov commented Mar 10, 2022

lcnr commented Mar 11, 2022

petrochenkov commented Mar 11, 2022

lcnr commented May 9, 2022 • edited Loading

petrochenkov commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

rust-timer commented May 10, 2022

update `hash_stable` for `List<Ty<'tcx>>` #94799

update `hash_stable` for `List<Ty<'tcx>>` #94799

lcnr commented Mar 10, 2022 •

edited

Loading

lcnr commented May 9, 2022 •

edited

Loading