Reduce the amount of interning and `layout_of` calls in const eval. #74202

oli-obk · 2020-07-09T22:28:31Z

If we just want to get at some bits of a constant, we don't need to intern it before extracting those bits.
Also, if we want to read a usize or bool, we can fetch the size without invoking a query.

oli-obk · 2020-07-09T22:28:42Z

@bors try @rust-timer queue

rust-timer · 2020-07-09T22:28:44Z

Awaiting bors try build completion

bors · 2020-07-09T22:28:54Z

⌛ Trying commit 60d032b3f389e3f34d175d0e71658b5ec5e3c85f with merge 5585d82a144a6885bbd224d4aa643c0c95d39fb6...

oli-obk · 2020-07-09T22:29:40Z

cc @rust-lang/wg-const-eval

bors · 2020-07-10T00:53:01Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 5585d82a144a6885bbd224d4aa643c0c95d39fb6 (5585d82a144a6885bbd224d4aa643c0c95d39fb6)

rust-timer · 2020-07-10T00:53:02Z

Queued 5585d82a144a6885bbd224d4aa643c0c95d39fb6 with parent 5db778a, future comparison URL.

rust-timer · 2020-07-10T04:01:21Z

Finished benchmarking try commit (5585d82a144a6885bbd224d4aa643c0c95d39fb6): comparison url.

oli-obk · 2020-07-10T06:32:41Z

booya 5% perf improvement

r? @RalfJung

oli-obk · 2020-07-10T06:33:02Z

@bors rollup=never

src/librustc_middle/mir/interpret/value.rs

RalfJung · 2020-07-11T07:18:20Z

src/librustc_middle/ty/sty.rs

-                Err(ErrorHandled::Reported(ErrorReported)) => tcx.const_error(self.ty),
+                Ok(val) => Some(Ok(val)),
+                Err(ErrorHandled::TooGeneric | ErrorHandled::Linted) => None,
+                Err(ErrorHandled::Reported(e)) => Some(Err(e)),


This looks pretty different from the old code, is all I can say here.^^ No idea if it changes behavior.

this is not the same function. The same function invokes this function adn handles all the cases just like they are handled here.

RalfJung · 2020-07-11T07:18:47Z

I have not seen most of this code ever before, so I do not think I can meaningfully review this. I don't even understand how this reduces interning or layout_of...
I know Miri well, but that is really restricted to the stuff in the intepret folders. Everything outside is code I barely ever touch or look at. And when I look at it I am usually confused. ;)

Wow, we have Const and ConstKind and ConstValue? That's way more confusing than I remembered. Particularly great is that Const is described as a "Typed constant value" but contains a ConstKind...

oli-obk · 2020-07-11T07:26:00Z

I don't even understand how this reduces interning or layout_of...

layout_of invocations are reduced by knowing that the size of usize and bool is tcx.data_layout.pointer_size and Size::from_bytes(1) respectively.

Interning is reduced by evaluating ConstValue instead of ty::Const where we just throw away the ty::Const anyway in order to get at inner values.

RalfJung · 2020-07-11T07:29:34Z

Interning is reduced by evaluating ConstValue instead of ty::Const where we just throw away the ty::Const anyway in order to get at inner values.

Which entry points, concretely, do you mean here? I tried to follow the code in the diff a bit but it is rather hard. There's a dozen functions on 2-3 different types that are all called the same, and when looking at a fn foo you cannot even tell which type self has.^^

src/librustc_middle/ty/sty.rs

oli-obk · 2020-07-11T07:30:52Z

src/librustc_middle/ty/sty.rs

    }

    #[inline]
    pub fn try_eval_usize(&self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Option<u64> {
-        self.try_eval_bits(tcx, param_env, tcx.types.usize).map(|v| v as u64)
+        self.val.eval(tcx, param_env).try_to_machine_usize(tcx)


reduces interning

This also reduces layout_of invocations, because instead of invoking that on tcx.types.usize, we just use the size in try_to_machine_usize

src/librustc_middle/ty/sty.rs

oli-obk · 2020-07-11T07:34:18Z

src/librustc_middle/ty/sty.rs


+impl<'tcx> Const<'tcx> {
    #[inline]
    pub fn try_eval_bool(&self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Option<bool> {


This is used in a mir optimization on every single Assert terminator:

rust/src/librustc_mir/transform/simplify_branches.rs

Line 53 in f9fdf64

} if (c.literal.try_eval_bool(tcx, param_env) == Some(true)) == expected => {

oli-obk · 2020-07-11T07:34:48Z

src/librustc_middle/ty/sty.rs

-            1 => Some(true),
-            _ => None,
-        })
+        self.val.eval(tcx, param_env).try_to_bool()
    }

    #[inline]
    pub fn try_eval_usize(&self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Option<u64> {


This is used all over the place for array lengths

RalfJung · 2020-07-11T07:41:03Z

src/librustc_middle/ty/sty.rs

@@ -2333,14 +2333,41 @@ impl<'tcx> Const<'tcx> {
        assert_eq!(self.ty, ty);
        let size = tcx.layout_of(param_env.with_reveal_all().and(ty)).ok()?.size;
        // if `ty` does not depend on generic parameters, use an empty param_env
-        self.eval(tcx, param_env).val.try_to_bits(size)
+        self.val.eval(tcx, param_env).try_to_bits(size)
    }

    #[inline]
    /// Tries to evaluate the constant if it is `Unevaluated`. If that doesn't succeed, return the
    /// unevaluated constant.
    pub fn eval(&self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> &Const<'tcx> {


Oh so on Const we have try_eval_bits and eval but they do not call each other, and there is neither eval_bits nor try_eval. But try_eval exists on ConstKind. But it is not used to implement Const::try_eval_bits, at least not directly... I feel I need to draw a call graph by hand to understand what happens.

RalfJung · 2020-07-11T07:54:44Z

src/librustc_middle/ty/consts/kind.rs

+/// Represents a constant in Rust.
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, RustcEncodable, RustcDecodable, Hash)]
+#[derive(HashStable)]
+pub enum ConstKind<'tcx> {


Longer-term I feel like this is really the thing that ought to be called Const, and the existing Const should become ConstAndTy or so... but that is for a separate PR.

src/librustc_middle/ty/consts/kind.rs

src/librustc_middle/ty/consts.rs

bors · 2020-07-15T17:06:59Z

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

RalfJung · 2020-07-15T17:42:45Z

r=me after rebasing.

oli-obk · 2020-07-16T09:20:07Z

@bors r=RalfJung

bors · 2020-07-16T09:20:08Z

📌 Commit 1bf0993 has been approved by RalfJung

bors · 2020-07-16T10:18:29Z

⌛ Testing commit 1bf0993 with merge 125c58c...

bors · 2020-07-16T13:34:58Z

☀️ Test successful - checks-actions, checks-azure
Approved by: RalfJung
Pushing 125c58c to master...

nnethercote · 2020-07-21T00:15:08Z

The perf results from landing don't quite match the earlier results. It's still a win for wf-projection-stress-65510, but it's up to a 2% loss for clap-rs, and a fraction of 1% loss for numerous benchmarks.

@oli-obk: did anything change between the earlier run and landing that might explain these worse results?

oli-obk · 2020-07-21T09:07:34Z

The only thing could be some changes around inlining due to the restructuring of the modules, but I don't think this should have an effect within a single crate. If I click the red percentages to look at the details, these regressions aren't actually reflected in the detailed view (or I don't know how to read the detailed view).

mati865 · 2020-07-21T09:18:03Z

encode_query_results_for is where the regression is coming from.

oli-obk · 2020-07-21T10:00:57Z

While there's indeed a 1.6% regression in encode_query_results_for, there's a 40% improvement because we now stop invoking the typeck_tables_of query. https://perf.rust-lang.org/detailed-query.html?commit=125c58caebc67c32ec45ac6c0581b596fd532082&base_commit=4cd0ee9343da86d9770bf0a514a682d240e0dce8&benchmark=clap-rs-check&run_name=incr-unchanged shows nothing of the 2% regression that is shown in the main view

oli-obk · 2020-07-21T10:02:18Z

Ah, no I see now. Incremental loading and execution are separated. And apparently loading typeck_tables_of takes 4.0% more time than computing it 👀

mati865 · 2020-07-21T10:59:26Z

typeck_tables_of time was marginal so improvement's here aren't terribly beneficial.
encode_query_results_for alone takes 1/3 of the total time so when you add it's 1.6% regression to other smaller regressions it starts to become visible in the final result.

rust-highfive assigned RalfJung Jul 10, 2020