rustdoc DocFragment rework #80261

GuillaumeGomez · 2020-12-21T12:55:58Z

Kind of a follow-up of #80119.

A few things are happening in this PR. I'm not sure about the impact on perf though so I'm very interested about that too (if the perf is worse, then we can just close this PR).

The idea here is mostly about reducing the memory usage by relying even more on Symbol instead of String. The only issue is that DocFragment has 3 modifications performed on it:

Unindenting
Collapsing similar comments into one
"Beautifying" (weird JS-like comments handling).

To do so, I saved the information about unindent and the "collapse" is now on-demand (which is why I'm not sure the perf will be better, it has to be run multiple times...).

r? @jyn514

jyn514

I think this should be separated into two changes: one refactoring rustdoc to work on demand, and one refactoring rustc_ast to work on Symbol instead of String. That way we can measure the performance of each more accurately, and it means that I can assign a rustc reviewer without the discussion getting intermingled. Do you mind opening a separate PR for that?

compiler/rustc_ast/src/util/comments.rs

compiler/rustc_save_analysis/src/lib.rs

src/librustdoc/clean/types.rs

src/librustdoc/html/render/cache.rs

src/librustdoc/html/render/mod.rs

src/librustdoc/passes/collapse_docs.rs

src/librustdoc/passes/collect_intra_doc_links.rs

GuillaumeGomez · 2020-12-21T21:03:56Z

Time to check if this approach is the right one!

@bors try @rust-timer queue

rust-timer · 2020-12-21T21:03:57Z

Awaiting bors try build completion.

bors · 2020-12-21T21:44:57Z

⌛ Trying commit 0022dfacdc697de78b17bbb298d4719df366df32 with merge 9ef3226338bac4bd516f19a1000cd1c4e741a039...

bors · 2020-12-21T22:30:59Z

☀️ Try build successful - checks-actions
Build commit: 9ef3226338bac4bd516f19a1000cd1c4e741a039 (9ef3226338bac4bd516f19a1000cd1c4e741a039)

rust-timer · 2020-12-21T22:31:01Z

Queued 9ef3226338bac4bd516f19a1000cd1c4e741a039 with parent 11c94a1, future comparison URL.

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

rust-timer · 2020-12-22T00:13:14Z

Finished benchmarking try commit (9ef3226338bac4bd516f19a1000cd1c4e741a039): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

GuillaumeGomez · 2020-12-22T10:53:54Z

Results of the perf check:

rss: -0.8%
instructions: between -0.4% and -1.8%

So it seems to be a good result. Should we continue on this @jyn514 ?

…rochenkov Rework beautify_doc_string so that it returns a Symbol instead of a String This commit comes from rust-lang#80261, the goal here is to inspect the impact on performance of this change on its own. The idea of rewriting `beautify_doc_string` is to not go through `String` if we don't need to update the doc comment to be able to keep the original `Symbol` and also to have better performance. r? `@jyn514`

GuillaumeGomez · 2020-12-24T17:55:58Z

Updated!

src/librustdoc/clean/types.rs

jyn514 · 2020-12-24T18:17:06Z

src/librustdoc/clean/types.rs

+        let s: String = self.doc_strings.iter().collect();
+        if s.is_empty() { None } else { Some(s) }


This is still waiting for a response: where is this used and when would the caller expect it to be None?

src/librustdoc/core.rs

src/librustdoc/html/render/mod.rs

src/librustdoc/passes/calculate_doc_coverage.rs

src/librustdoc/passes/collapse_docs.rs

src/librustdoc/passes/unindent_comments/tests.rs

GuillaumeGomez · 2020-12-28T16:22:08Z

Updated! ping @jyn514 ;)

jyn514 · 2021-01-02T18:16:09Z

src/test/rustdoc-ui/invalid-syntax.stderr

+LL |   ///     \____/
+   |  _________^
+LL | | ///
+   | |_


This change looks strange, do you know why it happened?

There is a slight difference in the markdown sent to passes::source_span_for_markdown_range.

Before this PR: --> 24..30 "```\nlet x = 1;\n```\n\n \\____/" With this PR: --> 24..31 "```\nlet x = 1;\n```\n\n \\____/\n"

So as you can see, we have an extra backline added now.

Explanations about his change: in impl<'a> FromIterator<&'a DocFragment> for String {, before we simply collapsed the strings together and added a backline if the "accumulator" wasn't empty. However, we now take into account if a DocFragment requires a backline or not which is then added if necessary. This information is stored in the need_backline field and computed in the Attributes::from_ast method.

So in this case, since this is an indented codeblock, it's a different fragment kind than the one following it, which then adds the backline in the ouput. Here's what the doc fragments looks like:

-> DocFragment { line: 0, span: src/test/rustdoc-ui/invalid-syntax.rs:90:1: 90:8 (#0), parent_module: None, doc: " ```", kind: SugaredDoc, need_backline: true, indent: 1 } -> DocFragment { line: 1, span: src/test/rustdoc-ui/invalid-syntax.rs:91:1: 91:15 (#0), parent_module: None, doc: " let x = 1;", kind: SugaredDoc, need_backline: true, indent: 1 } -> DocFragment { line: 2, span: src/test/rustdoc-ui/invalid-syntax.rs:92:1: 92:8 (#0), parent_module: None, doc: " ```", kind: SugaredDoc, need_backline: true, indent: 1 } -> DocFragment { line: 3, span: src/test/rustdoc-ui/invalid-syntax.rs:93:1: 93:4 (#0), parent_module: None, doc: "", kind: SugaredDoc, need_backline: true, indent: 0 } -> DocFragment { line: 3, span: src/test/rustdoc-ui/invalid-syntax.rs:94:1: 94:15 (#0), parent_module: None, doc: " \\____/", kind: SugaredDoc, need_backline: true, indent: 1 } -> DocFragment { line: 4, span: src/test/rustdoc-ui/invalid-syntax.rs:95:1: 95:4 (#0), parent_module: None, doc: "", kind: SugaredDoc, need_backline: false, indent: 0 } warning: could not parse code block as Rust code

Wow, that is confusing.

Ok, adding a trailing newline seems like the correct behavior because there's a blank doc-comment after the code block:

rust/src/test/rustdoc-ui/invalid-syntax.rs

Lines 88 to 93 in df2df14

/// ```

/// let x = 1;

/// ```

///

/// \____/

///

. I'm not 100% why the error message changes because of that, but the new error message is reasonable too so I don't think it needs to be debugged.

jyn514 · 2021-01-03T03:51:47Z

@bors r+

bors · 2021-01-03T03:51:49Z

📌 Commit df2df14 has been approved by jyn514

bors · 2021-01-03T06:29:48Z

⌛ Testing commit df2df14 with merge e821a6e...

bors · 2021-01-03T09:16:17Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing e821a6e to master...

`src/test/rustdoc-ui/deprecated-attrs.rs` tells rustdoc to run the `collapse-docs` pass, which no longer exists (it was removed in rust-lang#80261). Rustdoc doesn't actually give a proper diagnostic here; instead it prints an `error!` log. Now that tracing is compiled unconditionally, the log is now being emitted by default, because it's at the error level. rustdoc shouldn't be using `error!` logging for diagnostics in the first place, but in the meantime this change gets the testsuite to pass.

rust-highfive assigned jyn514 Dec 21, 2020