Revert parts of #66405.

[nnethercote]

Because PR #66405 caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes #67454.

r? @nikomatsakis

[nnethercote]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 694e59388332d3e360c998b1494360cb8a04269e with merge 1f56c6236b5b488c73c83200b59b74c26868a66b...

[bors]

☀️ Try build successful - checks-azure
Build commit: 1f56c6236b5b488c73c83200b59b74c26868a66b (1f56c6236b5b488c73c83200b59b74c26868a66b)

[rust-timer]

Queued 1f56c6236b5b488c73c83200b59b74c26868a66b with parent ccd2383, future comparison URL.

[rust-timer]

Finished benchmarking try commit 1f56c6236b5b488c73c83200b59b74c26868a66b, comparison URL.

[nikomatsakis]

Hmm, the perf results don't seem too reflect much improvement?

[Mark-Simulacrum]

We've had trouble with the perf results on this PR historically - the perf server is not quite accepting of the wins (or I guess losses too) that @nnethercote manages to measure locally. We're not sure why, though we've mostly eliminated bugs in the perf server itself such as measuring different artifacts.

It might be an architecture difference (e.g., @nnethercote is not using AMD chips) but that would be quite surprising to me.

[nnethercote]

This PR is expected to be a small performance loss on the benchmarks, because it reverts #66405, which was a small performance gain on the benchmarks, mostly for inflate and keccak.

But as @Mark-Simulacrum says, the perf results were very strange for that PR: inconsistent across multiple CI runs, all of which didn't really match my local results. (To the point where I did an experiment in #67248 to make sure the right revisions were being tested; they were.) And the results above are equally inconsistent, with as many wins as there are losses. And I have no idea why unicode_normalization has the largest change, it shouldn't be affected by ObligationForest changes. The whole thing is a mess.

Anyway, overall I think we should land this because it has minimal effect on the benchmark suite, but is known to fix some catastrophic performance regressions for real-world programs outside the benchmark suite (in #67454 and #67331). We should also backport this to beta, so that no rustc release contains the regression.

I won't have a chance to analyze what went wrong in #66405 until I get back from PTO in late January, unfortunately.

One could also argue that the test program in #67454 should be added to the benchmark suite to prevent future regressions of this sort.

[sdroege]

One could also argue that the test program in #67454 should be added to the benchmark suite to prevent future regressions of this sort.

Can code in the benchmarks depend on external crates? If so I can prepare a PR for this.

[nnethercote]

Can code in the benchmarks depend on external crates? If so I can prepare a PR for this.

They can, and most of them do!

[sdroege]

Perfect, took me a while to find the right repo but here it is: rust-lang/rustc-perf#588 :)

[sdroege]

What's blocking this to be merged?

[nikomatsakis]

Nothing, sorry, r=me -- but needs rebase

[memoryruins]

From #67454 (comment)

I'm about to go on vacation for 6 weeks, so I will file a PR to back out #66405 before I go.

Since they return ~a day before the next release, someone else with permissions might need to handle the rebase.

[Mark-Simulacrum]

I will rebase and r=niko shortly.

[Mark-Simulacrum]

@bors r=nikomatsakis rollup=never

[bors]

📌 Commit d9f14507f6150078407a466acf48d497d8ad8711 has been approved by nikomatsakis

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-31T22:34:04.8387293Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-31T22:34:05.6919091Z ##[command]git config gc.auto 0
2019-12-31T22:34:05.6924634Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-31T22:34:05.6926816Z ##[command]git config --get-all http.proxy
2019-12-31T22:34:05.6933149Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67471/merge:refs/remotes/pull/67471/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[Mark-Simulacrum]

@bors r=nikomatsakis rollup=never

[bors]

📌 Commit 18a3669 has been approved by nikomatsakis

[bors]

⌛ Testing commit 18a3669 with merge e380efa...

[bors]

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing e380efa to master...

[fengalin]

Many thanks, compilation times for this use case are back to normal on nightly now.

Friendly reminder, @nnethercote commented:

We should also backport this to beta, so that no rustc release contains the regression

[pnkfelix]

discussed at T-compiler meeting. Accepted for beta-backport.