Cache const eval queries #48864

oli-obk · 2018-03-09T06:10:27Z

fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants)

r? @michaelwoerister

oli-obk · 2018-03-09T06:12:41Z

@bors try

(we should run real perf tests first I guess)

bors · 2018-03-09T06:12:50Z

⌛ Trying commit 34e40835466881add360ade69d2c1be23ddc2cb5 with merge 1096950c850a1c21d1e8a5b1caf6593da10997bf...

bors · 2018-03-09T08:02:47Z

☀️ Test successful - status-travis
State: approved= try=True

oli-obk · 2018-03-09T08:19:34Z

ping @simulacrum can you run a perf test?

oli-obk · 2018-03-09T10:49:22Z

hmm... while this works and passes tests, it can cause AllocIds for statics to exist without a corresponding Allocation when running with incremental, which causes an ICE.

Mark-Simulacrum · 2018-03-09T12:56:07Z

Perf started for 1096950c850a1c21d1e8a5b1caf6593da10997bf.

oli-obk · 2018-03-09T13:24:34Z

Okay. I figured it out. incremental caches only works for queries that take a single DefId. You can get the query to get serialized, but the inverse won't work (and there's no compile-time error 😢 ).

So now I implemented it from scratch.

oli-obk · 2018-03-12T10:03:00Z

This should now improve incremental builds compared to pre-miri. Not just undo the regression.

michaelwoerister · 2018-03-12T11:31:34Z

src/librustc/ty/maps/config.rs

+
+    #[inline]
+    fn cache_on_disk(key: Self::Key) -> bool {
+        key.value.instance.def_id().is_local()


Can non-local consts be loaded from metadata? Otherwise we just might want to cache everything.

not yet (at least not in general, some are), but it is planned

Let's just return true here then.

michaelwoerister · 2018-03-12T11:33:35Z

src/librustc/ty/maps/on_disk_cache.rs

@@ -223,6 +231,45 @@ impl<'sess> OnDiskCache<'sess> {
                encode_query_results::<trans_fn_attrs, _>(tcx, enc, qri)?;
            }

+            // encode successful constant evaluations
+            let constants_index = {


It would be great if we had a comment (here or elsewhere) explaining why constants even need special handling (like you did on IRC).

michaelwoerister · 2018-03-12T11:34:04Z

@bors try

bors · 2018-03-12T11:34:14Z

⌛ Trying commit e2a375426584ebaa8359d598a9ea947057a539eb with merge a04ce41981aa3b98cd562106e77e2a7c0761883c...

bors · 2018-03-12T13:28:54Z

☀️ Test successful - status-travis
State: approved= try=True

michaelwoerister · 2018-03-12T14:18:48Z

@Mark-Simulacrum, could you do a perf-run please?

oli-obk · 2018-03-13T08:18:44Z

We could reuse the regular caching strategy, if either

we don't cache const_eval queries that resulted in an error (which does not necessarily result in a reported error!, so we might end up redoing some work in the next compilation)
we don't serialize the error enum and instead pull a dummy variant from the ether during deserialization (does this cause problems with hashing/equality expecting the same value after deserialization?)

The reason for this is that serializing/deserializing the error enum is a huge mess... We'd need to serialize things like ty::Layout and others, because the error variants contain such types.

Mark-Simulacrum · 2018-03-13T13:50:43Z

Perf should be queued.

michaelwoerister · 2018-03-13T14:06:09Z

Thanks, @Mark-Simulacrum!

http://perf.rust-lang.org/compare.html?start=222b0eb583a64299f43d6640617e22f19973ddab&end=a04ce41981aa3b98cd562106e77e2a7c0761883c&stat=instructions%3Au

michaelwoerister · 2018-03-13T14:23:18Z

We could reuse the regular caching strategy, if (...) we don't cache const_eval queries that resulted in an error

That sounds fine to me but how is it related to the problems with statics that we talked about yesterday? Is this an additional problem?

oli-obk · 2018-03-13T14:56:40Z

That sounds fine to me but how is it related to the problems with statics that we talked about yesterday? Is this an additional problem?

I'd change statics to always return ByRef instead of ByVal, so they contain an AllocId to their allocation. Simple constants will stay ByVal. Since the query caching will then cache the ByRef, it'll also trigger a serialization for the AllocId.

I don't think anyone is reading the value of statics at compile time atm other than const eval itself, so having it ByRef is fine. This is only a convenience feature for reading the constant's value, there's no disadvantage other than that accessing a ByRef constant requires a bunch of function calls instead of just matching on the value.

michaelwoerister · 2018-03-13T15:29:37Z

OK, sounds good!

@Mark-Simulacrum, I think that perf-run is unfortunately obsolete then.

michaelwoerister · 2018-03-13T15:42:45Z

@bors try

bors · 2018-03-13T15:42:56Z

⌛ Trying commit 0d88db1 with merge 0209427...

@michaelwoerister

Cache const eval queries fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants) r? @michaelwoerister

bors · 2018-03-13T17:46:11Z

☀️ Test successful - status-travis
State: approved= try=True

Mark-Simulacrum · 2018-03-14T01:03:41Z

Perf queued.

ishitatsuyuki · 2018-03-14T02:56:09Z

Compare with merge base: https://perf.rust-lang.org/compare.html?start=e65547d4fad0425d1db4f33a4d8134bf2cad939e&end=02094271f28e88e64b66014726f1d996bf7acae3&stat=instructions%3Au

Compare with the day before regression: https://perf.rust-lang.org/compare.html?start=2018-03-08&end=02094271f28e88e64b66014726f1d996bf7acae3&stat=instructions%3Au

oli-obk · 2018-03-14T05:29:26Z

Style servo regressed in clean incremental, but not in patched incremental. Weird. I probably won't get to investigate this before monday

Everything else gained compared to before miri. Now there are mostly regular compile-time regressions, not incremental regressions.

ishitatsuyuki · 2018-03-14T05:30:52Z

@oli-obk servo style clean-incr regression is known to be spurious; you can check the graph and it's widely fluctuating.

michaelwoerister · 2018-03-14T10:57:59Z

OK, I think we should merge this version.

@eddyb, would you mind taking a look at the const evaluator changes?

eddyb · 2018-03-14T13:17:30Z

@bors r+

bors · 2018-03-14T13:17:31Z

📌 Commit 0d88db1 has been approved by eddyb

kennytm · 2018-03-14T15:20:36Z

@bors p=1

Slightly bumping performance improvement PRs.

@michaelwoerister

Cache const eval queries fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants) r? @michaelwoerister

bors · 2018-03-14T15:40:06Z

⌛ Testing commit 0d88db1 with merge 11d9959...

bors · 2018-03-14T18:18:45Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 11d9959 to master...

rust-highfive assigned michaelwoerister Mar 9, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2018

oli-obk force-pushed the miri_incremental_regression branch from 34e4083 to 6cdc8cf Compare March 9, 2018 13:24

oli-obk force-pushed the miri_incremental_regression branch 2 times, most recently from 4e566ec to e2a3754 Compare March 12, 2018 10:00

michaelwoerister reviewed Mar 12, 2018

View reviewed changes

oli-obk added 2 commits March 13, 2018 15:57

Cache const eval queries

af099bb

Reuse the query caching infrastructure for const eval

0d88db1

oli-obk force-pushed the miri_incremental_regression branch from e2a3754 to 0d88db1 Compare March 13, 2018 15:24

bors added a commit that referenced this pull request Mar 13, 2018

Auto merge of #48864 - oli-obk:miri_incremental_regression, r=<try>

0209427

Cache const eval queries fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants) r? @michaelwoerister

eddyb approved these changes Mar 14, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2018

bors added a commit that referenced this pull request Mar 14, 2018

Auto merge of #48864 - oli-obk:miri_incremental_regression, r=eddyb

11d9959

Cache const eval queries fixes #48846 (I think, still running more perf tests, but tuple-stress stops recomputing any constants) r? @michaelwoerister

bors merged commit 0d88db1 into rust-lang:master Mar 14, 2018

bors mentioned this pull request Mar 14, 2018

WIP: Parallelize passes using rayon #46564

Closed

oli-obk mentioned this pull request Mar 16, 2018

Constant evaluation error at wrong location #40604

Closed

oli-obk deleted the miri_incremental_regression branch June 15, 2020 15:26

Cache const eval queries #48864

Cache const eval queries #48864

Conversation

oli-obk commented Mar 9, 2018 • edited Loading

oli-obk commented Mar 9, 2018

bors commented Mar 9, 2018

bors commented Mar 9, 2018

oli-obk commented Mar 9, 2018

oli-obk commented Mar 9, 2018

Mark-Simulacrum commented Mar 9, 2018

oli-obk commented Mar 9, 2018

oli-obk commented Mar 12, 2018

michaelwoerister Mar 12, 2018

Choose a reason for hiding this comment

oli-obk Mar 13, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 13, 2018

Choose a reason for hiding this comment

michaelwoerister Mar 12, 2018

Choose a reason for hiding this comment

michaelwoerister commented Mar 12, 2018

bors commented Mar 12, 2018

bors commented Mar 12, 2018

michaelwoerister commented Mar 12, 2018

oli-obk commented Mar 13, 2018

Mark-Simulacrum commented Mar 13, 2018

michaelwoerister commented Mar 13, 2018

michaelwoerister commented Mar 13, 2018

oli-obk commented Mar 13, 2018

michaelwoerister commented Mar 13, 2018

michaelwoerister commented Mar 13, 2018

bors commented Mar 13, 2018

bors commented Mar 13, 2018

Mark-Simulacrum commented Mar 14, 2018

ishitatsuyuki commented Mar 14, 2018

oli-obk commented Mar 14, 2018

ishitatsuyuki commented Mar 14, 2018

michaelwoerister commented Mar 14, 2018

eddyb commented Mar 14, 2018

bors commented Mar 14, 2018

kennytm commented Mar 14, 2018

bors commented Mar 14, 2018

bors commented Mar 14, 2018

oli-obk commented Mar 9, 2018 •

edited

Loading