Optimize layout of TypeVariants #50395
Merged
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thank you, @Zoxc! Looks beautiful! :)
You should definitely add a regression test that makes sure that TypeVariants stays at 24 bytes. Would be a shame if it grew again (without good reason).
I've added some comments to the first commit. The other commits look good to me. It would be great though, if someone else could take a look at the GeneratorSubsts change, since I don't know that code too well.
src/librustc/lib.rs
Outdated
| @@ -40,6 +40,7 @@ | |||
| html_favicon_url = "https://doc.rust-lang.org/favicon.ico", | |||
| html_root_url = "https://doc.rust-lang.org/nightly/")] | |||
|
|
|||
| #![feature(asm)] | |||
There was a problem hiding this comment.
It not. I've removed it.
src/librustc/ty/context.rs
Outdated
| let i = ($alloc_to_ret)(self.global_interners.arena.$alloc_method(v)); | ||
| let i: &$lt_tcx $ty = $alloc_method(&self.global_interners.arena, v); | ||
| // Cast to 'gcx | ||
| let i = unsafe { mem::transmute(i) }; |
There was a problem hiding this comment.
Making this let i: &'gcx $ty = ... would make it even more explicit.
There was a problem hiding this comment.
That doesn't work since $ty uses 'tcx lifetimes, hence the need for transmute.
There was a problem hiding this comment.
I didn't mean that the transmute should be removed, rather that the binding is explicitly annotated for more clarity:
let i: &$lt_tcx $ty = $alloc_method(&self.global_interners.arena, v);
// Cast to 'gcx
let i: &'gcx $ty = unsafe { mem::transmute(i) };
There was a problem hiding this comment.
I think @Zoxc's point is that $ty already has explicit lifetimes in it, so it wouldn't work.
OTOH, since this is a reference, maybe pointer casting should be used?
There was a problem hiding this comment.
I can probably just refactor the macros to get rid of the transmute here.
src/librustc/ty/mod.rs
Outdated
| @@ -566,18 +568,75 @@ impl <'gcx: 'tcx, 'tcx> Canonicalize<'gcx, 'tcx> for Ty<'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| extern { | |||
| type Unsized; | |||
There was a problem hiding this comment.
This could have a more descriptive name, such as OpaqueSliceContents.
There was a problem hiding this comment.
I've renamed it to that.
src/librustc/ty/mod.rs
Outdated
| // Align up the size of the len (usize) field | ||
| let align = mem::align_of::<T>(); | ||
| let align_mask = align - 1; | ||
| let offset = mem::size_of::<usize>(); |
There was a problem hiding this comment.
A u32 for the length would probably be enough in practice.
There was a problem hiding this comment.
Perhaps. I don't think we have many 4-byte aligned types in slices though.
There was a problem hiding this comment.
That's true. Nevermind then.
src/librustc/ty/mod.rs
Outdated
|
|
||
| let mem: *mut u8 = &mut arena.alloc_raw( | ||
| size, | ||
| cmp::max(mem::align_of::<T>(), mem::align_of::<usize>()))[0]; |
There was a problem hiding this comment.
You could use .as_mut_ptr() here, I think.
src/librustc/ty/mod.rs
Outdated
| &self.0 | ||
| unsafe { | ||
| if self as *const _ as usize == EMPTY_SLICE { | ||
| return mem::transmute(slice::from_raw_parts(mem::align_of::<T>() as *const T, 0)) |
There was a problem hiding this comment.
You might be able to just return &[] here?
|
@bors p=1 (this is likely to bitrot) |
|
r? @eddyb (for reviewing the commit that introduces |
kennytm
reviewed
May 2, 2018
src/librustc/util/common.rs
Outdated
| // Trigger a debugger if we crashed during bootstrap | ||
| DebugBreak(); | ||
| } | ||
| eprintln!("PANIC HOOK END"); |
Zoxc
force-pushed
the
small-tys
branch
2 times, most recently
from
119916b
to
9973dea
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
May 3, 2018
| @@ -888,9 +888,10 @@ for ty::TypeVariants<'gcx> | |||
| TyRawPtr(pointee_ty) => { | |||
| pointee_ty.hash_stable(hcx, hasher); | |||
| } | |||
| TyRef(region, pointee_ty) => { | |||
| TyRef(region, pointee_ty, mutbl) => { | |||
There was a problem hiding this comment.
Why not also change TyRawPtr and maybe getting rid of TypeAndMut?
There was a problem hiding this comment.
Because it's additional work which does not further reduce the size of TypeVariants. It probably would be a bit cleaner though.
|
So this PR looks relatively okay (the generator parts especially). |
|
I've removed the commit for thin slices. It caused linking errors after I rebased this PR. |
That's weird. Did the commit introduce some instability in symbol hashes? |
|
@bors try |
bors
added a commit
that referenced
this pull request
May 3, 2018
Optimize layout of TypeVariants This makes references to `Slice` use thin pointers by storing the slice length in the slice itself. `GeneratorInterior` is replaced by storing the movability of generators in `TyGenerator` and the interior witness is stored in `GeneratorSubsts` (which is just a wrapper around `&'tcx Substs`, like `ClosureSubsts`). Finally the fields of `TypeAndMut` is stored inline in `TyRef`. These changes combine to reduce `TypeVariants` from 48 bytes to 24 bytes on x86_64. r? @michaelwoerister
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum I'd like a perf run here |
kennytm
added
the
S-waiting-on-perf
|
The perf results show runtime improved in some cases and degraded in others. It's mostly noise, I'd say. |
|
Some sort of memory usage benchmark would be interesting for this. |
kennytm
removed
the
S-waiting-on-perf
|
@bors r=michaelwoerister |
|
📌 Commit c9d9c24 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit c9d9c24 with merge 25a79f6b346ed8e0c43d313fab38a206b06677e3... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
May 10, 2018
Optimize layout of TypeVariants This makes references to `Slice` use thin pointers by storing the slice length in the slice itself. `GeneratorInterior` is replaced by storing the movability of generators in `TyGenerator` and the interior witness is stored in `GeneratorSubsts` (which is just a wrapper around `&'tcx Substs`, like `ClosureSubsts`). Finally the fields of `TypeAndMut` is stored inline in `TyRef`. These changes combine to reduce `TypeVariants` from 48 bytes to 24 bytes on x86_64. r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
|
📣 Toolstate changed by #50395! Tested on commit 0a223d1. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
kennytm-githubbot
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
May 10, 2018
Tested on commit rust-lang/rust@0a223d1. Direct link to PR: <rust-lang/rust#50395> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
This was referenced May 10, 2018
bors
added a commit
that referenced
this pull request
May 22, 2018
Make &Slice a thin pointer Split out from #50395 r? @michaelwoerister
bors
added a commit
that referenced
this pull request
May 28, 2018
Make &Slice a thin pointer Split out from #50395 r? @michaelwoerister
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.
This makes references to
Sliceuse thin pointers by storing the slice length in the slice itself.GeneratorInterioris replaced by storing the movability of generators inTyGeneratorand the interior witness is stored inGeneratorSubsts(which is just a wrapper around&'tcx Substs, likeClosureSubsts). Finally the fields ofTypeAndMutis stored inline inTyRef. These changes combine to reduceTypeVariantsfrom 48 bytes to 24 bytes on x86_64.r? @michaelwoerister