Store const_eval_raw results to disk

[Zoxc]

Based on #59722.

r? @oli-obk

[Zoxc]

@oli-obk Do you know why we only store const eval results which does not result in an error?

[Zoxc]

@bors try

[oli-obk]

Do you know why we only store const eval results which does not result in an error?

I don't remember. I think I just kept the status quo alive and assumed there's a reason.

[oli-obk]

We do need this for statics, because const_eval will validate statics, and statics can be mutually recursive. So we call const_eval_raw to not run validation during const eval.

This will increase the amount of cached data, potentially by very large amounts. Every constant that has a trivial representation via ConstValue::Scalar will now require a full allocation to be cached to disk.

But I think I can design a perf test that shows that this PR is a perf improvement, but I'm not sure if its worth it to include it. Essentially it should be

// lib.rs of crate `foo`
static FOO: u32 = 42; // do some very complex static computation here

// main.rs of crate `foo`
static BAR: u32 = *&foo::FOO + 1;

Because reading from a static via an indirection will trigger

rust/src/librustc_mir/interpret/memory.rs

Line 394 in 7e08576

let raw_const = tcx.const_eval_raw(ty::ParamEnv::reveal_all().and(gid))

The only other uses are const evaluation of const fn with promoteds:

rust/src/librustc_mir/interpret/place.rs

Line 564 in 7e08576

self.const_eval_raw(GlobalId {

Ok, I just found one use that is odd:

rust/src/librustc_mir/interpret/operand.rs

Line 525 in d3e2cec

return Ok(OpTy::from(self.const_eval_raw(GlobalId {

introduced in ba82f54 @RalfJung since we only need an operand here, could we do the recursive thing again and use const_eval and call const_to_op instead of using const_eval_raw.

Then we could cache const_eval_raw only for promoteds and statics, although we could consider having promoteds go thorough the const_eval path, too.

[RalfJung]

I did that because (a) converting a ConstValue back to an Operand is very messy, because ConstValue is really optimized for whatever the rest of the compiler needs, and I had hoped that we will eventually just kill the function in const_eval.rs that performs this conversion and the world will be a better place, and (b) I prefer to consistently use the same query for the constants/statics we encounter during evaluation.

[RalfJung]

Ah, turns out that messy ConstValue-to-op code is in that same function eval_const_to_op. ;) So I guess we cannot avoid that mess. :/

The argument about consistency remains though, and also I'd expect a performance cost from doing validation there when it is not needed?

Why is this call not a problem? That one cannot be replaced. Right now all "recursive" const eval goes through InterpCtx::const_eval_raw and that's nice.

rust/src/librustc_mir/interpret/place.rs

Line 564 in d3e2cec

self.const_eval_raw(GlobalId {

[oli-obk]

I'd expect a performance cost from doing validation there when it is not needed?

That cost will be paid anyway once const_eval is called (which it is for all constants)

So I guess we cannot avoid that mess. :/

We could avoid it if we removed ConstValue::Scalar and ConstValue::Slice, but that has a performance regression (I tried it once)

The reason I want to remove it is so that we don't store the Allocation of a trivial constant in metadata when we start caching const_eval_raw

[RalfJung]

That cost will be paid anyway once const_eval is called (which it is for all constants)

Well, except for generic consts (once they exist). But I get what you are saying.

The reason I want to remove it is so that we don't store the Allocation of a trivial constant in metadata when we start caching const_eval_raw

Remove what? I don't understand how that is connected to replacing one of three places where the interpreter calls const_eval_raw, with const_eval -- why doing something else sometimes should be of any help.

[oli-obk]

why doing something else sometimes should be of any help.

If we only cache const_eval_raw calls for statics and promoteds, but we keep calling it for constants, then downstream crates will recompute those constants instead of fetching them from the cache. So if we use const_eval for constants, we only need to cache const_eval, which we already do.

[Zoxc]

@bors try

[Zoxc]

@bors try

[bors]

⌛ Trying commit 6a5afdd with merge c385f72...

[RalfJung]

If we only cache const_eval_raw calls for statics and promoteds, but we keep calling it for constants, then downstream crates will recompute those constants instead of fetching them from the cache. So if we use const_eval for constants, we only need to cache const_eval, which we already do.

So you want to reliably use const_eval_raw for statics/promoteds, but const_eval for constants? I see. I don't like this lack of uniformity, but at least it's not entirely arbitrary -- and I guess there's always some price to pay for performance.

Do you think we can set things up in a way such that if we ever get it wrong, we get an ICE? That would put me more at ease with this scheme.

[bors]

💥 Test timed out

[bors]

☔ The latest upstream changes (presumably #59722) made this pull request unmergeable. Please resolve the merge conflicts.

[Zoxc]

@bors try

[bors]

⌛ Trying commit 77c0af6 with merge 6ae40628b538d3adb52b1770f7eb908a19255148...

[bors]

💔 Test failed - checks-azure

[Zoxc]

@bors try

[bors]

⌛ Trying commit 77c0af6 with merge a60befa583852dee37603e3bd00fd04ff2870b54...

[Zoxc]

@rust-timer build a60befa583852dee37603e3bd00fd04ff2870b54

[rust-timer]

Success: Queued a60befa583852dee37603e3bd00fd04ff2870b54 with parent 765eebf, comparison URL.

[bors]

💥 Test timed out

[rust-timer]

Finished benchmarking try commit a60befa583852dee37603e3bd00fd04ff2870b54, comparison URL.