Revert lazy init

[Mark-Simulacrum]

It looks like the implementation of the perf-tests for rollups is subtly wrong: it used the tree of the rollup merge, which included all rollup merges prior to that (rather than just the benchmarked PR). #74611 resolved to a neutral perf result which means that it was not the regression in #74468. Furthermore, results from #74592 now give a fairly strong indication that the problem is indeed the lazy initialization PR (#72414).

I suspect that the root cause here is the same as the gimli PR -- adding a sizeable amount of code to libstd increases overheads across the board.

cc @nnethercote

[rust-highfive]

r? @kennytm

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

[Mark-Simulacrum]

@eddyb -- we'll likely want to re-land the niche-filling PR, but I'd like to do so once we've resolved the current perf state fully just in case (though I now believe it to be perf-neutral).

@bors try @rust-timer queue

r? @nnethercote

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 6eaaff4 with merge 193b2f77c9463ed7378c20bad843a9031489e215...

[Mark-Simulacrum]

I expect the results here to roughly speaking be a perfect inverse of https://perf.rust-lang.org/compare.html?start=d3df8512d2c2afc6d2e7d8b5b951dd7f2ad77b02&end=cfade73820883adf654fe34fd6b0b03a99458a51, by the way.

[matklad]

Hm, yeah, it is pretty unfortunate that an addition of std API which is not even used anywhere yet measurably regresses benchmarks.

Proceduraly, do we want to block this until the compiler is fixed to deal with unused code faster? It might be reasonable in terms of end-user experience. But, this is really not fixable within the PR itself I think, which is not good either.

In terms of overall story with compile-times, I am actually hoping that OnceCellwill reduce them long term, by replacing lazy_static! across the ecosystem with something which is precompiled as part of stdlib, and which doesn't generated swaths of code with macros.

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 193b2f77c9463ed7378c20bad843a9031489e215 (193b2f77c9463ed7378c20bad843a9031489e215)

[rust-timer]

Queued 193b2f77c9463ed7378c20bad843a9031489e215 with parent 9008693, future comparison URL.

[Mark-Simulacrum]

Oh, I think it's unclear whether we'll land this PR or not. I think probably yes but we may re-land the lazy init PR without fixing the compiler, just like we're somewhat likely to do with the backtrace stuff.

I agree with you 100% that it is pretty annoying that just adding an API to std regresses performance for downstream crates despite lack of use.

[Mark-Simulacrum]

I am quite confused by these results. Let me summarize what we've run so far:

• The original rollup, Rollup of 11 pull requests #74468, landed with a 5-10% performance regression
• In Revert "Compare tagged/niche-filling layout and pick the best one" #74611, we tested as-if a rollup of Compare tagged/niche-filling layout and pick the best one #74069 and Add lazy initialization primitives to std #72414 landed. (This is the diff between d3df85 and cfade73) That yielded identical results to the rollup.
• We reverted Compare tagged/niche-filling layout and pick the best one #74069 in Revert "Compare tagged/niche-filling layout and pick the best one" #74611, this yielded neutral performance results.
• This PR then reverted Add lazy initialization primitives to std #72414 but this also yielded neutral performance results

These results are pretty much nonsensical, so I attempted to reproduce them locally. I was unable to do so, at least on the helloworld and unify-linearly benchmarks -- I observe no performance difference in the original rollup.

I then investigated what occurred on the collector between these two benchmarks.

2020-07-18 02:32:46.688688+00 is when d3df851 was collected.
2020-07-20 09:33:46.351706+00 is when 7d31ffc was collected.

There are several differences that landed in that time range onto the collection server.

We stopped building std on the collection server: rust-lang/rustc-perf@048360c. This was deployed on 2020-07-19 14:34:00, with 47ea6d9 being the first commit benchmarked since then.

There were likely some other deploys and changes that I'll try to include later. I haven't yet figured out if we should assign all blame to the lack of std recompilation or what else, yet, though. It doesn't help that the collector benchmarks things out of order with respect to git history. This is a log of the commits and when they were benchmarked on the collection server.

[rust-timer]

Finished benchmarking try commit (193b2f77c9463ed7378c20bad843a9031489e215): 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

[Mark-Simulacrum]

Having thought about this some more, it seems quite likely that having std built locally was giving us some performance benefits, though I'm not sure why -- it seems like we tried pretty hard to get a 1-1 matching there.

I have re-queued d3df851, the parent of the rollup in question to run. My hope is that once that finishes, we'll see that the perf results of the rollup (saved to https://perf.rust-lang.org/compare.html?start=d3df8512d2c2afc6d2e7d8b5b951dd7f2ad77b02-v1&end=7d31ffc1ac9e9ea356e896e63307168a64501b9d&stat=instructions:u) will be neutral, rather than a regression.

If that's the case, then we'll probably just chalk this up to "everything's okay" and drop this revert. I'll also re-land #74069 since that'll mean it is indeed performance neutral (which is almost certain, given the results of its revert).

If my theory is right, I expect that rebuilding d3df851 on perf will show that it "regressed" by approximately equal amounts as the rollup did. It would be good to then pull in @eddyb (since they wrote the original std-in-perf code) to try and see if we can come up with a reason for CI std to be a performance regression. (Maybe we can land something fixing CI std for everyone!).

[nnethercote]

I'll also re-land #74069

Please do a CI perf run first :)

[Mark-Simulacrum]

I am closing this PR as our investigation has led us to believe that the rollup in question, #74468, is not a regression and is indeed performance neutral. We've also updated the triage log to account for this and document what happened.