Shrink ParamEnv to 16 bytes #73978
Shrink ParamEnv to 16 bytes #73978
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue I personally suspect that the overhead on access may erase some of the wins here, but it's unclear -- depends on if we copy the values around more or access the fields more, I guess. |
|
Awaiting bors try build completion |
|
⌛ Trying commit 11c802b024d95e2a9d88c4a204046367b1352286 with merge a5a9f964fe2e8c2722d433ff27446ff72bdd4cff... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued a5a9f964fe2e8c2722d433ff27446ff72bdd4cff with parent 3503f56, future comparison URL. |
|
Finished benchmarking try commit (a5a9f964fe2e8c2722d433ff27446ff72bdd4cff): comparison url. |
|
Looks like some reasonable wins. Adjusted the Hash impl to avoid pulling out the pointer and enum separately, just hashing the pointer directly, let's see if that helps improve the wins here a bit more. @bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit d6f2554111681111a745df0d90dd4029a1174a81 with merge 7f6d689a6a0a19aa8873a1034c27b49f859521e0... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 7f6d689a6a0a19aa8873a1034c27b49f859521e0 with parent f844ea1, future comparison URL. |
|
Finished benchmarking try commit (7f6d689a6a0a19aa8873a1034c27b49f859521e0): comparison url. |
Mark-Simulacrum
force-pushed
the
shrink-paramenv
branch
2 times, most recently
from
007e820 to
ffcbbe5
Compare
|
Looks like changing the Hash impl had roughly zero effect, which is not unexpected (hashing one extra byte was unlikely to really contribute any time), but seems like a reasonable thing to do regardless. I've polished up the code a bit; I think this is ready for review. It seems like perf shows (very) slight wins across the board. This is also likely to be a memory usage win, though that's not really showing up in our max-rss (unsurprisingly). r? @nnethercote |
src/librustc_middle/ty/mod.rs
Outdated
| pub struct ParamEnv<'tcx> { | ||
| // We pack the caller_bounds List pointer and a Reveal enum into this usize, relying | ||
| // on the List<ty::Predicate<'tcx>> type having at least 2-byte alignment. | ||
| // List's start with a usize and are repr(C) so this should be fine; there |
src/librustc_middle/ty/mod.rs
Outdated
| pub struct ParamEnv<'tcx> { | ||
| // We pack the caller_bounds List pointer and a Reveal enum into this usize, relying |
There was a problem hiding this comment.
Nit: explain more clearly what the packing is, i.e. the low bit is the reveal (where 0 means UserFacing and 1 means All) and the rest is the pointer (which works because of the > 2-byte alignment).
| packed_data: packed_data | ||
| | match reveal { | ||
| Reveal::All => 1, | ||
| Reveal::UserFacing => 0, |
There was a problem hiding this comment.
Nit: swap these arms so the 0 is first?
Mark-Simulacrum
force-pushed
the
shrink-paramenv
branch
from
ffcbbe5 to
8512d2e
Compare
|
@bors r=nnethercote rollup=never (due to perf effect) |
|
📌 Commit 8512d2e has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 |
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 8512d2e with merge 708f84e70f4726600b2dd3ce23b58dbf6cb98274... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
Seems like a legitimate timeout, but I don't really think this PR could be causing it, so let's @bors retry - timeout (mingw) again. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 8512d2e with merge c18e7b14eb1d045a640c926c3d6dfda5faeceb98... |
|
@bors retry yield |
|
Unfortunately timeouts on Azure are now common occurrence (usually with one of Windows jobs taking build above the time limit):
|
|
Yes, i've noticed the bors build timing out a lot more often :/ |
|
Already reported on zulip: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/ci.20timeouts |
|
☀️ Test successful - checks-actions, checks-azure |
|
This was a perf win, as expected. Nice work! |
…ethercote Shrink ParamEnv to 16 bytes r? @nnethercote x.py check passes but I haven't tried running perf or tests
| // We pack the caller_bounds List pointer and a Reveal enum into this usize. | ||
| // Specifically, the low bit represents Reveal, with 0 meaning `UserFacing` | ||
| // and 1 meaning `All`. The rest is the pointer. | ||
| // | ||
| // This relies on the List<ty::Predicate<'tcx>> type having at least 2-byte | ||
| // alignment. Lists start with a usize and are repr(C) so this should be | ||
| // fine; there is a debug_assert in the constructor as well. | ||
| // | ||
| // Note that the choice of 0 for UserFacing is intentional -- since it is the | ||
| // first variant in Reveal this means that joining the pointer is a simple `or`. | ||
| packed_data: usize, |
There was a problem hiding this comment.
Late to the party, but maybe move all of the safety-critical parts of this into a ty::param_env module? That or we could try to build a bit-packed abstraction (since ty::GenericArg does something similar), or look for one on crates.io and audit it.
r? @nnethercote
x.py check passes but I haven't tried running perf or tests