Revert the rustdoc box syntax removal #89134
Conversation
It turned out to cause (minor) perf regressions.
|
Some changes occurred in intra-doc-links. cc @jyn514 |
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors: r+ |
|
📌 Commit f809ed6 has been approved by |
bors
added
S-waiting-on-bors
…GuillaumeGomez Revert the rustdoc box syntax removal Reverts the rustdoc box syntax removal from rust-lang#87781. It turned out to cause (minor) perf regressions. Requested in rust-lang#87781 (comment)
|
since it's perf-relevant @bors rollup=never |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (cfff31b): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
|
This barely had any effect for any benchmark except two doc benchmarks which got up to 0.45% faster. |
|
For rustdoc we only look at doc benchmarks, so I guess it's good. :) |
|
Great, the number in the improvement matches the number in the regression (also up to 0.45%, but negative): #87781 (comment) I had the fear that this PR wasn't enough and more of #87781 had to be reverted as e.g. rustdoc would specially use some regressing component in compiler/ that the compiler itself made no heavy use of... but thankfully that wasn't the case. Yeah the perf regression is minimal, and maybe a revert wouldn't have been needed, but I don't want to regress performance even by a little bit by removing box syntax usage from the compiler. The question is always what you gain from it, and box syntax removal is not worth "much" in my eyes, at least at this point when it's not clear if box syntax will go or maybe even get stabilized one day. Anyways, 90% of #87781 hasn't been reverted (check the diff numbers, this PR is roughly a tenth of #87781), and it's pretty easy to remove the remainder from rustdoc one day should the need arise. |
|
Thanks for that! Even though it was a small regression, we've been trying to go the other way for quite some time now so every improvement is very welcome (even when it's reverting a regression ;) ). |
|
This only partially fixes the original regression of the doc benchmarks: https://perf.rust-lang.org/compare.html?start=896f058f13d6c8021f7637817953a44d3a78be32&end=ba8cda2fa2c99ed6646f4dfe73bf4edad7e42a2d This fixes serde-doc and stm32f4-doc, but doesn't fix many-assoc-items-doc, match-stress-enum-doc, unused-warnings-doc and the less significant regressions derive-doc, match-stress-exhaustive_patterns-doc, style-servo-doc and syn-doc. |
|
@bjorn3 where is the unused-warnings-doc regression? I can't see it. |
|
Uncheck "Filter out very small changes". It was a 0.28% regression. |
|
@bjorn3 oh right, there they are. If you check that checkbox in this PR's perf run though, you'll see that all these tests had improvements from this PR, so I think this PR fixes them all. |
Remove most box syntax from librustdoc This is the second attempt after the librustdoc specific changes have been reverted from rust-lang#87781 in rust-lang#89134, due to a minor, but exant regression caused by the changes. ~~There have been some changes to librustdoc in the past and maybe thanks to them there is no regression any more. If there is still a regression, one can investigate further and maybe find ways to fix the regressions. Thus, i request a perf run.~~ Edit: turns out there is still a regression, but it's caused only by a subset of the changes. So I've changed this PR to only contains the changes that don't cause any performance regressions, keeping the regression causing changes for a later PR.
Reverts the rustdoc box syntax removal from #87781.
It turned out to cause (minor) perf regressions.
Requested in #87781 (comment)