-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't transmute &List<GenericArg>
<-> &List<Ty>
#110496
The head ref may contain hidden characters: "\u{1F3F3}\uFE0F\u200D\u26A7\uFE0Fsound"
Conversation
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 10ec03c with merge 6156b93350a5f3c622e7eec405ce70b7f72da35f... |
&List<GenericArg>
<-> &List<Ty>
(sorry, i like the title but it has nothing to do with what the pr's actually doing, so i changed it 😅) |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (6156b93350a5f3c622e7eec405ce70b7f72da35f): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
cool, r=me Seems from the PR that seldom in practice do we ever need to interconvert between |
what |
Well, that's cool with me, I'll cleanup & r=errs tomorrow. |
cc #94799 (comment) Seems like it may have already been understood that this transmute wasn't really saving much perf in reality. Seems like max-rss isn't affected either. Lgtm. |
@bors r=compiler-errors rollup- |
This comment was marked as resolved.
This comment was marked as resolved.
@bors rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (df0d9b4): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
…nsmutes, r=compiler-errors Remove an unused `&[Ty]` <-> `&[GenericArg]` Missed this one in rust-lang#110496, oops. r? `@compiler-errors`
…errors Stable hash tag (discriminant) of `GenericArg` This is a continuation of my quest of removing `transmute` if generic args and types (rust-lang#110496, rust-lang#110599). 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`
In #93505 we allowed safely transmuting between
&List<GenericArg<'_>>
and&List<Ty<'_>>
. This was possible becauseGenericArg
is a tagged pointer and the tag for types is0b00
, such that aGenericArg
with a type inside has the same layout asTy
.While this was meant as an optimization, it doesn't look like it was actually any perf or max-rss win (see #94799 (comment), #94841, #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 asrepr(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 thesetransmutes
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