Merge `DocContext.{ty,lt,ct}_substs` into one map #90443

camelid · 2021-10-31T17:21:36Z

It should be impossible to have more than one entry with a particular
key across the three maps, so they should be one map. In addition to
making it impossible for multiple entries to exist, this should improve
memory usage since now only one map is allocated on the stack and heap.

r? @GuillaumeGomez

camelid · 2021-10-31T17:23:18Z

It's probably best to review this commit-by-commit, and potentially with whitespace hidden.

camelid · 2021-10-31T17:24:29Z

src/librustdoc/clean/mod.rs

@@ -222,7 +222,7 @@ impl Clean<Lifetime> for hir::Lifetime {
            | rl::Region::Free(_, node_id),
        ) = def
        {
-            if let Some(lt) = cx.lt_substs.get(&node_id).cloned() {
+            if let Some(lt) = cx.substs.get(&node_id).and_then(|p| p.as_lt()).cloned() {


I could've made this unwrap the Option returned by as_lt since it's a rustdoc bug for it to be None here, but it seemed simpler to just use and_then. Let me know if you want me to use unwrap as a sort of assertion. (Likewise with as_ty below.)

I guess it's fine... Maybe add a FIXME on it so we can come back to it later on?

Let's either leave it as-is or switch it to unwrap. I don't think it's worth a FIXME either way, and it's not actionable.

camelid · 2021-10-31T17:25:44Z

@bors try @rust-timer queue

rust-timer · 2021-10-31T17:25:46Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-10-31T17:25:52Z

⌛ Trying commit 85d93c66d09ac8f793166ad190c1e3ba29cff528 with merge 57ef206d81386eab59b1c32b9e291ae1010c7f8e...

camelid · 2021-10-31T17:32:53Z

Hmm, somehow I left trailing whitespace...

It should be impossible to have more than one entry with a particular key across the three maps, so they should be one map. In addition to making it impossible for multiple entries to exist, this should improve memory usage since now only one map is allocated on the stack and heap.

bors · 2021-10-31T18:58:10Z

☀️ Try build successful - checks-actions
Build commit: 57ef206d81386eab59b1c32b9e291ae1010c7f8e (57ef206d81386eab59b1c32b9e291ae1010c7f8e)

rust-timer · 2021-10-31T18:58:12Z

Queued 57ef206d81386eab59b1c32b9e291ae1010c7f8e with parent 68b554e, future comparison URL.

rust-timer · 2021-10-31T22:01:02Z

Finished benchmarking commit (57ef206d81386eab59b1c32b9e291ae1010c7f8e): 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.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

GuillaumeGomez · 2021-11-01T18:49:35Z

There doesn't seem to have any memory usage to be displayed in the performance comparison... Kinda hard to judge if your PR has an impact on it haha.

GuillaumeGomez · 2021-11-01T18:49:56Z

Otherwise the code looks good to me, but I'd be really interested about the RSS usage change.

camelid · 2021-11-01T21:05:00Z

@GuillaumeGomez There's a max-rss comparison on the page rust-timer links to: https://perf.rust-lang.org/compare.html?start=68b554e6af18726fe6fa8de2134c59c441e0b019&end=57ef206d81386eab59b1c32b9e291ae1010c7f8e&stat=max-rss. There are some regressions for whatever reason, but it says they are all insignificant.

GuillaumeGomez · 2021-11-01T21:19:54Z

Oh right, I forgot it was "hidden". So that still seems surprising that the memory usage grows in here...

camelid · 2021-11-01T21:28:08Z

Oh right, I forgot it was "hidden". So that still seems surprising that the memory usage grows in here...

I wonder if it's because an enum is being stored, so there's an extra tag? We should probably find a way to get rid of these tables at some point too.

GuillaumeGomez · 2021-11-08T13:49:11Z

Looks good to me, thanks! We can look for a way to remove the tables in another PR.

@bors: r+

bors · 2021-11-08T13:49:13Z

📌 Commit 5a77f30 has been approved by GuillaumeGomez

camelid · 2021-11-08T17:42:01Z

@bors rollup=never (just in case the max-rss changes are real)

bors · 2021-11-08T21:38:03Z

⌛ Testing commit 5a77f30 with merge 5154727...

bors · 2021-11-09T00:29:14Z

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 5154727 to master...

rust-timer · 2021-11-09T02:00:12Z

Finished benchmarking commit (5154727): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

Moderate regression in instruction counts (up to 0.5% on full builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

camelid · 2021-11-09T02:03:30Z

The instruction count regressions that were reported are spurious—this change doesn't touch the compiler.

Good news: no significant max-rss changes, so the regression reported previously was probably spurious.

camelid · 2021-11-09T02:05:21Z

A bunch of very small instruction count improvements for doc builds though!

rylev · 2021-11-09T16:00:08Z

I'm adding back the perf-regression as its currently our only way of tracking interesting perf related cases. Don't worry about actually looking into this. The perf team will want to investigate why this change to rustdoc led to fairly significant changes in non-noisy test cases. We should not be spurious changes that are this large.

@rustbot label: +perf-regression

cc @Mark-Simulacrum

camelid added 2 commits October 31, 2021 10:19

Extract function for expanding private type aliases

eeb2a62

Reduce rightward drift

bbc58e8

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 31, 2021

rust-highfive assigned GuillaumeGomez Oct 31, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2021

camelid commented Oct 31, 2021

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 31, 2021

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 31, 2021

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 1, 2021

camelid force-pushed the rustdoc-subst branch from 85d93c6 to 5a77f30 Compare November 1, 2021 23:31

camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 2, 2021

GuillaumeGomez approved these changes Nov 8, 2021

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 Nov 8, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 9, 2021

bors merged commit 5154727 into rust-lang:master Nov 9, 2021

rustbot added this to the 1.58.0 milestone Nov 9, 2021

camelid deleted the rustdoc-subst branch November 9, 2021 01:01

rustbot added the perf-regression Performance regression. label Nov 9, 2021

camelid added I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. and removed I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. perf-regression Performance regression. labels Nov 9, 2021

rustbot added the perf-regression Performance regression. label Nov 9, 2021

Merge DocContext.{ty,lt,ct}_substs into one map #90443

Merge DocContext.{ty,lt,ct}_substs into one map #90443

Uh oh!

Conversation

camelid commented Oct 31, 2021

Uh oh!

camelid commented Oct 31, 2021

Uh oh!

camelid Oct 31, 2021

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

camelid Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

camelid commented Oct 31, 2021

Uh oh!

rust-timer commented Oct 31, 2021

Uh oh!

bors commented Oct 31, 2021

Uh oh!

This comment has been minimized.

camelid commented Oct 31, 2021

Uh oh!

bors commented Oct 31, 2021

Uh oh!

rust-timer commented Oct 31, 2021

Uh oh!

rust-timer commented Oct 31, 2021

Uh oh!

GuillaumeGomez commented Nov 1, 2021

Uh oh!

GuillaumeGomez commented Nov 1, 2021

Uh oh!

camelid commented Nov 1, 2021

Uh oh!

GuillaumeGomez commented Nov 1, 2021

Uh oh!

camelid commented Nov 1, 2021

Uh oh!

GuillaumeGomez commented Nov 8, 2021

Uh oh!

bors commented Nov 8, 2021

Uh oh!

camelid commented Nov 8, 2021

Uh oh!

bors commented Nov 8, 2021

Uh oh!

bors commented Nov 9, 2021

Uh oh!

rust-timer commented Nov 9, 2021

Uh oh!

camelid commented Nov 9, 2021

Uh oh!

camelid commented Nov 9, 2021

Uh oh!

rylev commented Nov 9, 2021

Uh oh!

Uh oh!

Merge `DocContext.{ty,lt,ct}_substs` into one map #90443

Merge `DocContext.{ty,lt,ct}_substs` into one map #90443