Fixing the performance regression of #76244 #76913

vandenheuvel · 2020-09-19T10:38:41Z

Issue #74865 suggested that removing the def_id field from ParamEnv would improve performance. PR #76244 implemented this change.

Generally, results were as expected: an instruction count decrease of about a percent. The instruction count for the unicode crates increased by about 3%, which @nnethercote speculated to be caused by a quirk of inlining or codegen. As the results were generally positive, and for chalk integration, this was also a step in the right direction, the PR was r+'d regardless.

However, wall-time performance results show a much larger performance degradation: 25%, as mentioned by @Mark-Simulacrum.

This PR, for now, reverts #76244 and attempts to find out, which change caused the regression.

rust-highfive · 2020-09-19T10:38:45Z

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

vandenheuvel · 2020-09-19T10:39:15Z

r? @jackh726

lcnr · 2020-09-19T10:57:09Z

@bors try @rust-timer queue

rust-timer · 2020-09-19T10:57:10Z

Awaiting bors try build completion

bors · 2020-09-19T10:57:21Z

⌛ Trying commit 64f98169e0dda32efd981d568cebddad45d0b3cf with merge 62832cf234653ae26c54abc4801db9b0698a0f86...

jackh726 · 2020-09-19T11:15:54Z

Do try builds ignore tidy? If not, it will fail.

lcnr · 2020-09-19T11:17:03Z

I think they ignore tidy

bors · 2020-09-19T11:41:51Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 62832cf234653ae26c54abc4801db9b0698a0f86 (62832cf234653ae26c54abc4801db9b0698a0f86)

rust-timer · 2020-09-19T11:41:52Z

Queued 62832cf234653ae26c54abc4801db9b0698a0f86 with parent 4e8a8b4, future comparison URL.

rust-timer · 2020-09-19T14:04:10Z

Finished benchmarking try commit (62832cf234653ae26c54abc4801db9b0698a0f86): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

jackh726 · 2020-09-19T14:49:01Z

Ok cool, reverting the DefId removal wins us back the regressions.

Mark-Simulacrum · 2020-09-19T15:20:16Z

Yeah, seems like maybe not all of it, but I think we should land this and separately explore why removing the DefId cost us so much performance on some benchmarks. (I guess maybe it would be fairly clear from the diff, but I'd need to view it locally I suspect).

vandenheuvel · 2020-09-19T20:23:49Z

The latest commit reintroduces the refactor for chalk mode, as these changes are still desired. At this point, the net effect of this PR essentially is to introduce dead code. A FIXME is added to remove the field.

Mark-Simulacrum · 2020-09-19T20:27:25Z

@bors try @rust-timer queue

Let's re-confirm perf.

rust-timer · 2020-09-19T20:27:27Z

Awaiting bors try build completion

bors · 2020-09-19T20:27:36Z

⌛ Trying commit 44920d01d523ae17e2da07ebe7d9a8c628cdadb8 with merge 7ccd8cf9305ba5c78cfd78c0fa454f18a20c4b16...

bors · 2020-09-19T21:10:35Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 7ccd8cf9305ba5c78cfd78c0fa454f18a20c4b16 (7ccd8cf9305ba5c78cfd78c0fa454f18a20c4b16)

rust-timer · 2020-09-19T21:10:37Z

Queued 7ccd8cf9305ba5c78cfd78c0fa454f18a20c4b16 with parent 59fb88d, future comparison URL.

compiler/rustc_ty/src/ty.rs

rust-timer · 2020-09-19T22:28:35Z

Finished benchmarking try commit (7ccd8cf9305ba5c78cfd78c0fa454f18a20c4b16): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

jackh726 · 2020-09-20T17:38:27Z

This is a seriously small diff. But LGTM. I don't know who wants to actually review/r+ this? @Mark-Simulacrum?

Mark-Simulacrum

r=me with field made private, unless you want to try the MaybeUninit thing as well.

Mark-Simulacrum · 2020-09-20T19:38:09Z

compiler/rustc_middle/src/ty/mod.rs

@@ -1745,6 +1745,9 @@ pub struct ParamEnv<'tcx> {
    ///
    /// Note: This is packed, use the reveal() method to access it.
    packed: CopyTaggedPtr<&'tcx List<Predicate<'tcx>>, traits::Reveal, true>,
+
+    /// FIXME: This field is not used, but removing it causes a performance degradation. See #76913.
+    pub unused_field: Option<DefId>,


I would like this to be made private.

Maybe we should also replace Option<DefId> here with something like MaybeUninit<u64>, which would never be initialized, and see if that's still enough to avoid the regression?

Private: good point!

About the different type: I'm not sure what a nice solution would be. MaybeUninit is not Eq, for example. I now wrapped an array of u8's to stress that the side (probably) should be 8 bytes. What do you think?

Mark-Simulacrum · 2020-09-20T22:15:35Z

Could you squash the commits down as well? I imagine the revert re-apply dance are no longer necessary, given the small diff here.

Let's kick off a try build to make sure the array doesn't perform worse. @bors try @rust-timer queue

rust-timer · 2020-09-20T22:15:37Z

Awaiting bors try build completion

bors · 2020-09-20T22:15:47Z

⌛ Trying commit 106b74ba1a5abe1b617b33be0b440ea065080666 with merge 3b689b941bcd1844d1fae487ae64b061bb4492c3...

bors · 2020-09-20T22:57:33Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 3b689b941bcd1844d1fae487ae64b061bb4492c3 (3b689b941bcd1844d1fae487ae64b061bb4492c3)

rust-timer · 2020-09-20T22:57:35Z

Queued 3b689b941bcd1844d1fae487ae64b061bb4492c3 with parent 1fd5b9d, future comparison URL.

rust-timer · 2020-09-21T00:44:49Z

Finished benchmarking try commit (3b689b941bcd1844d1fae487ae64b061bb4492c3): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

jackh726 · 2020-09-21T02:36:24Z

Latest perf looks worse.

Mark-Simulacrum · 2020-09-21T03:04:42Z

Okay, let's go back to the DefId and we can try to follow-up separately on optimizing further.

r=me with commits squashed so we don't have the back and forth in git history

vandenheuvel · 2020-09-21T11:19:19Z

@Mark-Simulacrum I think that this should do it.

lcnr · 2020-09-21T20:52:03Z

@bors r+ rollup=never

bors · 2020-09-21T20:52:05Z

📌 Commit ab83d37 has been approved by lcnr

bors · 2020-09-22T00:22:29Z

⌛ Testing commit ab83d37 with merge 4519845...

bors · 2020-09-22T02:37:58Z

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 4519845 to master...

vandenheuvel · 2020-09-22T13:57:36Z

This issue finds continuation in #77058.

rust-highfive assigned varkor Sep 19, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2020

rust-highfive assigned jackh726 and unassigned varkor Sep 19, 2020

vandenheuvel force-pushed the performance_debug branch 4 times, most recently from 29cdc76 to 44920d0 Compare September 19, 2020 20:11

jackh726 reviewed Sep 19, 2020

View reviewed changes

compiler/rustc_ty/src/ty.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Sep 20, 2020

View reviewed changes

Mark-Simulacrum assigned Mark-Simulacrum and unassigned jackh726 Sep 20, 2020

vandenheuvel force-pushed the performance_debug branch from 4c30bf8 to 106b74b Compare September 20, 2020 22:01

Add an unused field of type Option<DefId> to ParamEnv struct.

ab83d37

vandenheuvel force-pushed the performance_debug branch from 106b74b to ab83d37 Compare September 21, 2020 07:49

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 Sep 21, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2020

bors merged commit 4519845 into rust-lang:master Sep 22, 2020

rustbot added this to the 1.48.0 milestone Sep 22, 2020

vandenheuvel deleted the performance_debug branch September 22, 2020 13:46

This was referenced Sep 22, 2020

Code generation issue blocks the removal of an unused struct field #77058

Closed

Optimize IntRange::from_pat, then shrink ParamEnv #77257

Merged

Fixing the performance regression of #76244 #76913

Fixing the performance regression of #76244 #76913

Conversation

vandenheuvel commented Sep 19, 2020

rust-highfive commented Sep 19, 2020

vandenheuvel commented Sep 19, 2020

lcnr commented Sep 19, 2020

rust-timer commented Sep 19, 2020

bors commented Sep 19, 2020

jackh726 commented Sep 19, 2020

lcnr commented Sep 19, 2020

bors commented Sep 19, 2020

rust-timer commented Sep 19, 2020

rust-timer commented Sep 19, 2020

jackh726 commented Sep 19, 2020

Mark-Simulacrum commented Sep 19, 2020

vandenheuvel commented Sep 19, 2020

Mark-Simulacrum commented Sep 19, 2020

rust-timer commented Sep 19, 2020

bors commented Sep 19, 2020

bors commented Sep 19, 2020

rust-timer commented Sep 19, 2020

rust-timer commented Sep 19, 2020

jackh726 commented Sep 20, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Sep 20, 2020

Choose a reason for hiding this comment

vandenheuvel Sep 20, 2020

Choose a reason for hiding this comment

Mark-Simulacrum commented Sep 20, 2020

rust-timer commented Sep 20, 2020

bors commented Sep 20, 2020

bors commented Sep 20, 2020

rust-timer commented Sep 20, 2020

rust-timer commented Sep 21, 2020

jackh726 commented Sep 21, 2020

Mark-Simulacrum commented Sep 21, 2020

vandenheuvel commented Sep 21, 2020

lcnr commented Sep 21, 2020

bors commented Sep 21, 2020

bors commented Sep 22, 2020

bors commented Sep 22, 2020

vandenheuvel commented Sep 22, 2020