Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustdoc: use CSS containment to speed up render #102253

Merged
merged 2 commits into from
Sep 28, 2022
Merged

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Sep 25, 2022

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Containment

This affected layout a little and required adjustments to the CSS to keep spacing the same. In particular, the margins of adjacent items usually overlap with each other. However, when an item has contain: layout, any margins of child nodes push out the size of the item itself. This was making spacing between items a little too big. To solve that, I removed margins in some places: in particular for certain classes that often occur at the end of a details.rustdoc-toggle block, I removed their bottom margin. Generally, the margins provided by the next item down are sufficient.

Also remove an unnecessary margin-top on .code-header.

In particular this helps with the problem that rustdoc in some situations can generate giant HTML pages, which can crash a Chrome tab on typical modern hardware, for instance: https://docs.rs/iced-x86/1.16.0/iced_x86/code_asm/struct.CodeAssembler.html (26MB, 409k DOM nodes). This doesn't, of course, universally solve the problem, but it pushes out the boundary of the largest page rustdoc can produce without crashing a browser tab.

Demos:

https://rustdoc.crud.net/jsha/css-contain/std/string/struct.String.html
(warning: giant page, may crash a browser tab) https://rustdoc.crud.net/jsha/css-contain-icedx86/iced_x86/code_asm/struct.CodeAssembler.html

r? @notriddle

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Sep 25, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2022
@GuillaumeGomez
Copy link
Member

You're doing two different things in here: reducing margins and speeding up page load. The second one is a great improvement, the first one I'm not sure to understand why it was done. It makes the CSS a bit more complicated so I assume that there is a good reason behind it. Can you provide more information about it please?

@notriddle
Copy link
Contributor

notriddle commented Sep 25, 2022

The appearance of the page shouldn’t change. The reduced margins are supposed to make up for the way that contain: layout turns off margin collapsing.

@GuillaumeGomez
Copy link
Member

That seems quite easy to break then. :-/

@notriddle
Copy link
Contributor

Yeah, we need to fix it so that rustdoc doesn’t rely on margin collapsing so much.

@bors
Copy link
Contributor

bors commented Sep 26, 2022

☔ The latest upstream changes (presumably #102292) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@notriddle
Copy link
Contributor

tl;dr: if you update the sidebar-mobile-scroll.goml test case to fix the slightly changed numbers, and get rid of the nav.sidebar { contain } rule to fix the sidebar-source-code-display.goml test case, and fix the merge conflict, then r=me.

sidebar-mobile-scroll.goml

[ERROR] (line 9) Error: Evaluation failed: The following errors happened: [Expected `643` for property `pageYOffset`, found `645`]: for command `assert-window-property: {"pageYOffset": "643"}`
[ERROR] (line 24) Error: Evaluation failed: The following errors happened: [Expected `643` for property `pageYOffset`, found `645`]: for command `assert-window-property: {"pageYOffset": "643"}`
[ERROR] (line 31) Error: Evaluation failed: The following errors happened: [Expected `643` for property `pageYOffset`, found `645`]: for command `assert-window-property: {"pageYOffset": "643"}`

Your version

image

Current nightly

image

It looks like, in the current nightly, there's a margin-top set on the header, and it's collapsing with the margin-bottom on the toggle. In your version, however, they don't collapse.

If the change is this small, then I don't really care. Just update the test case and move on. It's literally two pixels difference, it looks fine either way, and while you are adding a bit more space to the page, it's spacing added to the global sections, which aren't very numerous.

sidebar-source-code-display.goml

[ERROR] (line 230) Error: The following CSS properties still don't match: [width: (`0px` != `500px`)]: for command `wait-for-css: (".sidebar", {"width": "500px"})`

This one's a bit more concerning. Is the nav.sidebar { contain } rule actually necessary to see the performance improvements in Chrome? If not, this can probably be done in another PR.

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Containment

This affected layout a little and required adjustments to the CSS to
keep spacing the same. In particular, the margins of adjacent items
usually overlap with each other. However, when an item has contain:
layout, any margins of child nodes push out the size of the item itself.
This was making spacing between items a little too big. To solve that, I
removed margins in some places: in particular for certain classes that
often occur at the end of a `details.rustdoc-toggle` block, I removed
their bottom margin. Generally, the margins provided by the next item
down are sufficient.

Also remove an unnecessary margin-top on .code-header.
@rust-log-analyzer

This comment has been minimized.

margin-bottom: 2em;
}

.implementors-toggle[open] {
.implementors-toggle[open]:not(:last-child) {
margin-bottom: 2em;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 2em margin works out to be 32px.

This means that, by disabling it on the last child, you're reducing the margin on the bottom of a list of implementors in this scenario (on current nightly, it'll be 32px, while on your branch, it's now 25px, because that's the top margin of the header).

Co-authored-by: Michael Howell <michael@notriddle.com>
@notriddle
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 27, 2022

📌 Commit b5b77a2 has been approved by notriddle

It is now in the queue for this repository.

@bors 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 Sep 27, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2022
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#101555 (Stabilize `#![feature(mixed_integer_ops)]`)
 - rust-lang#102253 (rustdoc: use CSS containment to speed up render)
 - rust-lang#102281 (make invalid_value lint a bit smarter around enums)
 - rust-lang#102284 (Structured suggestion for missing `mut`/`const` in raw pointer)
 - rust-lang#102330 (rustdoc: remove no-op CSS `.srclink { font-weight; font-size }`)
 - rust-lang#102337 (Avoid LLVM-deprecated `Optional::hasValue`)
 - rust-lang#102356 (session: remove now-unnecessary lint `#[allow]`s)
 - rust-lang#102367 (rustdoc: remove redundant `#help-button` CSS)
 - rust-lang#102369 (Fix search result colors)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4cef648 into rust-lang:master Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants