always back ty::Const by an Allocation
#58486
Conversation
rust-highfive
added
the
S-waiting-on-review
oli-obk
changed the title
ty::Const ty an Allocationty::Const by an Allocation
This comment has been minimized.
This comment has been minimized.
| @@ -2065,7 +2066,7 @@ pub enum LazyConst<'tcx> { | |||
| } | |||
|
|
|||
| #[cfg(target_arch = "x86_64")] | |||
| static_assert!(LAZY_CONST_SIZE: ::std::mem::size_of::<LazyConst<'static>>() == 56); | |||
| static_assert!(LAZY_CONST_SIZE: ::std::mem::size_of::<LazyConst<'static>>() == 80); | |||
There was a problem hiding this comment.
And this isn't a problem? Let's do a perf run.
|
@bors try |
always back `ty::Const` by an `Allocation` This reduces the number of conversion between `Allocation` and `Value` by caching both. r? @RalfJung
|
☀️ Test successful - checks-travis |
|
@rust-timer build dc96b91 |
|
Success: Queued dc96b91 with parent f058741, comparison URL. |
|
Finished benchmarking try commit dc96b91 |
|
max-rss regressed noticeably, mostly for tuple-stress. |
| @@ -647,15 +646,15 @@ where | |||
| // does not permit code that would break this! | |||
| if self.alloc_map.contains_key(&alloc) { | |||
| // Not yet interned, so proceed recursively | |||
| self.intern_static(alloc, mutability)?; | |||
| let _alloc = self.intern_static(alloc, mutability)?; | |||
There was a problem hiding this comment.
Why bind this to an unused variable?
| let field = ecx.mplace_field(down, field.index() as u64)?; | ||
| let val = match field.layout.abi { | ||
| layout::Abi::Scalar(..) => { | ||
| let scalar = ecx.try_read_immediate_from_mplace(field)?.unwrap().to_scalar()?; |
There was a problem hiding this comment.
Couldn't you use try_read_immediate(field.into())? Then we could avoid exposing that internal function try_read_immediate_from_mplace.
| }; | ||
| let ptr = Pointer::from(constant.alloc_id); | ||
| let alloc = constant.alloc; | ||
| Ok(ty::Const { val, ty: mplace.layout.ty, alloc: Some((alloc, ptr))}) |
There was a problem hiding this comment.
The code above here is basically mplace_to_const, right? And we have the same in const_field, don't we? Couldn't that become a helper function?
| })(); | ||
| result.map_err(|error| { | ||
| let err = error_to_const_error(&ecx, error); | ||
| // FIXME(oli-obk): I believe this is unreachable and we can just ICE here. Since a constant | ||
| // is checked for validity before being in a place that could pass it to `const_field`, | ||
| // we can't possibly have errors. All fields have already been checked. |
There was a problem hiding this comment.
Could/should this FIXME be ported to the other PR?
There was a problem hiding this comment.
I'm trying this out locally in a different branch, so there's no reason to add it to the source tree
Const to op simplification r? @RalfJung alternative to rust-lang#58486
Const to op simplification r? @RalfJung alternative to rust-lang#58486
Const to op simplification r? @RalfJung alternative to rust-lang#58486
Const to op simplification r? @RalfJung alternative to rust-lang#58486
|
☔ The latest upstream changes (presumably #58691) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I believe this got superseded by #58511 and can be closed? |
This reduces the number of conversion between
AllocationandValueby caching both.r? @RalfJung