Store static initializers in metadata instead of the MIR of statics. #116564
Conversation
|
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…, r=<try>
Store static initializers in metadata instead of the MIR of statics.
This means that adding generic statics would be even more difficult, as we can't evaluate statics from other crates anymore, but the subtle issues I have encountered make me think that having this be an explicit problem is better. Said issues are:
### Nested allocations of static items get duplicated
which leads to issues for statics like
```rust
static mut FOO: &mut u32 = &mut 42;
static mut BAR = unsafe { FOO };
```
getting different allocations, instead of referring to the same one. This is also true for non-static mut, but promotion makes `static FOO: &u32 = &42;` annoying to demo.
### The main allocation of a static gets duplicated across crates
```rust
// crate a
static mut FOO: Option<u32> = Some(42);
// crate b
static mut BAR: &mut u32 = unsafe {
match &mut a::FOO {
Some(x) => x,
None => panic!(),
}
};
```
## Why is this being done?
In order to ensure all crates see the same nested allocations (which is the last issue that needs fixing before we can stabilize [`const_mut_refs`](rust-lang#57349)), I am working on creating anonymous (from the Rust side, to LLVM it's like a regular static item) static items for the nested allocations in a static. If we evaluate the static item in a downstream crate again, we will end up duplicating its nested allocations (and in some cases, like the `match` case, even duplicate the main allocation).
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Is that something that happens today or happens in your branch? |
| trace!("eval_to_allocation: Need to compute {:?}", gid); | ||
| let raw_const = self.eval_to_allocation_raw(param_env.and(gid))?; | ||
| Ok(self.global_alloc(raw_const.alloc_id).unwrap_memory()) | ||
| let alloc_id = self.eval_static_initializer_raw(def_id)?; |
There was a problem hiding this comment.
Calling eval_to_allocation to eval a static would be wrong now, right?
Should we hide eval_to_allocation better? It should probably only be called internally by the eval_const queries now?
There was a problem hiding this comment.
I'm adding asserts right now 😆 CI is failing because of "mis-use" (after this PR it would be mis-use) of the other queries for statics
I'm currently trying to figure it out, but it needs a multi-crate test |
|
Finished benchmarking commit (ca76eb8): comparison URL. Overall result: ❌✅ regressions and improvements - 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @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.
Binary sizeResultsThis 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.
Bootstrap: 626.784s -> 625.386s (-0.22%) |
rustbot
added
perf-regression
|
Ok, so I think it can't happen, but bugs in the interpreter could make it happen. Basically the reason it can't happen is that we always use the static's alloc id that gives us a
are all fine. The only potential issue came from |
|
oof that's a harsh regression in incremental-unchanged for crates with lots of static items. Seems like encoding all that extra data is expensive. |
I think you just re-did the analysis I did for #61345. :)
Can't we stop encoding other data now, in exchange? |
Oh yea whoops. I'm trying to get to the place that you described (returning Allocation instead of AllocId). Seems doable |
The issue here is not metadata encoding, from which I indeed removed the MIR encoding. The issue is the incremental cache. So I'm trying to stop calling the eval to allocation query from the eval static initializer query, which should hopefully eliminate this issue entirely by actually having fewer total queries called |
|
I'm not sure if those ideas from years ago are still good ideas today.^^ It's worth a try though.
But also see the [recent discussion on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/HashStable.20for.20AllocId.3F/near/394394970) about AllocId and StableHash... If we can get rid of AllocId in tcx entirely that would of course also solve the problem. Seems hard to do without bad perf impact though (and has some hard design questions as well).
|
I reinvented your ideas from trying to make the implementation do what I need it to, so seems good we arrived at the same place |
We have quite a few queries for CTFE that often just dispatch to a slightly different lower-level query... I do wonder if that's not just a bunch of unnecessary overhead. Do these all have to be queries with a full cache and everything? |
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…, r=RalfJung,cjgillot
Store static initializers in metadata instead of the MIR of statics.
This means that adding generic statics would be even more difficult, as we can't evaluate statics from other crates anymore, but the subtle issue I have encountered make me think that having this be an explicit problem is better.
The issue is that
```rust
static mut FOO: &mut u32 = &mut 42;
static mut BAR = unsafe { FOO };
```
gets different allocations, instead of referring to the same one. This is also true for non-static mut, but promotion makes `static FOO: &u32 = &42;` annoying to demo.
Fixes rust-lang#61345
## Why is this being done?
In order to ensure all crates see the same nested allocations (which is the last issue that needs fixing before we can stabilize [`const_mut_refs`](rust-lang#57349)), I am working on creating anonymous (from the Rust side, to LLVM it's like a regular static item) static items for the nested allocations in a static. If we evaluate the static item in a downstream crate again, we will end up duplicating its nested allocations (and in some cases, like the `match` case, even duplicate the main allocation).
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
oli-obk
force-pushed
the
evaluated_static_in_metadata
branch
from
d500a17
to
debb4cb
Compare
…n of a static. Instead we re-use the static's alloc id within the interpreter for its initializer to refer to the `Allocation` that only exists within the interpreter.
oli-obk
force-pushed
the
evaluated_static_in_metadata
branch
from
debb4cb
to
73b38c6
Compare
|
@bors r=RalfJung,cjgillot |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (6a4222b): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @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.
Binary sizeResultsThis 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.
Bootstrap: 635.116s -> 635.436s (0.05%) |
|
@rustbot label: +perf-regression-triaged The stress test now writes an 8MB constant to metadata instead of a tiny bit of MIR |
rustbot
added
the
perf-regression-triaged
This means that adding generic statics would be even more difficult, as we can't evaluate statics from other crates anymore, but the subtle issue I have encountered make me think that having this be an explicit problem is better.
The issue is that
gets different allocations, instead of referring to the same one. This is also true for non-static mut, but promotion makes
static FOO: &u32 = &42;annoying to demo.Fixes #61345
Why is this being done?
In order to ensure all crates see the same nested allocations (which is the last issue that needs fixing before we can stabilize
const_mut_refs), I am working on creating anonymous (from the Rust side, to LLVM it's like a regular static item) static items for the nested allocations in a static. If we evaluate the static item in a downstream crate again, we will end up duplicating its nested allocations (and in some cases, like thematchcase, even duplicate the main allocation).