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

Improve page load performance in rustdoc #82315

Merged
merged 2 commits into from
Mar 4, 2021
Merged

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Feb 20, 2021

Add an explicit height to icons (which already had an explicit width) to allow browsers to lay out the page more accurately before the icons have been loaded. https://web.dev/optimize-cls/.

Add min-width: 115px to the crate search dropdown. When the HTML first loads, this dropdown includes only the text "All crates." Later, JS loads the items underneath it, some of which are wider. That causes the dropdown to get wider, causing a distracting reflow. This sets a min-width based on the size that the dropdown eventually becomes based on the crates on doc.rust-lang.org, reducing page movement during load.

Add font-display: swap. Per https://web.dev/font-display/, this prevents "flash of invisible text" during load by using a system font until the custom font is available. I've noticed this flash of invisible text occasionally when reading Rust docs. Note that users without cached fonts will see text, and then see it reflow. For docs.rust-lang.org, setting caching headers will help a lot.

Generated output at https://jacob.hoffman-andrews.com/rust/flow-improvements/std/string/struct.String.html.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ollie27 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2021
@jsha jsha changed the title Use font-display: swap and icon height in rustdoc Improve page load performance in rustdoc Feb 20, 2021
@the8472
Copy link
Member

the8472 commented Feb 20, 2021

Inline the styles for custom fonts so the browser can start loading them earlier.

Wouldn't <link rel=preload ...> do the job too?

@GuillaumeGomez
Copy link
Member

Overall, it's all very nice changes. My only concern here is about inling the font css: I don't see much point for it and it'll make all HTML pages heavier for a small gain, if any.

@jsha
Copy link
Contributor Author

jsha commented Feb 20, 2021

I'm happy to switch to link preload. Here's the rationale I wrote in a commit (and then forgot to hoist to the PR desc, whoops):

This is slightly preferable to <link rel="preload" as="font" ...> because in the case that the user has the font installed locally, they can skip the network call altogether.

But it is indeed a slight benefit. I suspect most people don't have those fonts installed. I'll push a preload variant.

@jsha
Copy link
Contributor Author

jsha commented Feb 20, 2021

@GuillaumeGomez
Copy link
Member

My concern remains. ;)

@jsha
Copy link
Contributor Author

jsha commented Feb 20, 2021

My concern remains. ;)

As in, the <link> tags are unnecessary weight? Currently they are 664 bytes. The HTML for https://doc.rust-lang.org/std/string/struct.String.html is 573,186 bytes. The HTML for https://doc.rust-lang.org/std/primitive.u64.html is 525,379 bytes.

Here's why I think it's useful: Fonts are two levels deep in the page load hierarchy. The browser has to load the CSS before it even knows what URLs to fetch for the fonts. This leads to inefficient utilization of the network - the browser could have sent out requests for CSS and fonts in parallel, but now it has to load them in serial (relative to the CSS). It's a gain for the first load, though I think what you're getting at is that it may not be much of a gain for loads when the fonts are already in a disk cache?

A couple of resources. Unfortunately neither directly addresses the issue of how helpful preloading is when the resource is already on disk.

https://web.dev/preload-critical-assets/
https://web.dev/optimize-webfont-loading/

@jsha
Copy link
Contributor Author

jsha commented Feb 20, 2021

Also worth mentioning #82286: static resources on doc.rust-lang.org currently don't have cache headers and get a round-trip with a 304 on every page load. That's easily fixed for doc.rust-lang.org, but other sites hosting rustdoc might have the same problem (which is a web server config problem rather than a rustdoc problem). For sites that have this problem, preloading does improve performance even if the fonts are on disk already.

@Nemo157
Copy link
Member

Nemo157 commented Feb 20, 2021

Add font-display: swap.

IIRC font-display changes were previously tried in #72092 and #72557, then reverted in #72610, it'd be good to check whether the rationale from those still apply? (I don't personally know much about how the different settings interact, just bringing this up in case others that do have insights, maybe cc @workingjubilee).

@GuillaumeGomez
Copy link
Member

Currently they are 664 bytes. The HTML for https://doc.rust-lang.org/std/string/struct.String.html is 573,186 bytes. The HTML for https://doc.rust-lang.org/std/primitive.u64.html is 525,379 bytes.

It's maybe "only" 664 bytes, but it has to be multiplied by the number of pages, and there are a lot.

To be noted for later: in case we decide to go on with this CSS font rule inlining, let's remove the CSS font file and simply create a function in the source code to get the CSS font rules directly. No need to keep a file around if it's only to be used at compile time.

@notriddle
Copy link
Contributor

notriddle commented Feb 20, 2021

So, in other words, something like this?

fn generates_fonts_css() -> String {
    use std::fmt::Write;
    let fonts = [
        ("Fira Sans", "normal", "400", "Fira Sans", "FiraSans-Regular.woff"),
        ("Fira Sans", "normal", "500", "Fira Sans Medium", "FiraSans-Medium.woff"),
        ("Source Serif Pro", "normal", "400", "Source Serif Pro", "SourceSerifPro-Regular.ttf.woff"),
        ("Source Serif Pro", "italic", "400", "Source Serif Pro Italic", "SourceSerifPro-It.ttf.woff"),
        ("Source Serif Pro", "normal", "700", "Source Serif Pro Bold", "SourceSerifPro-Bold.ttf.woff"),
        ("Source Code Pro", "normal", "400", "", "SourceCodePro-Regular.ttf.woff"),
        ("Source Code Pro", "normal", "600", "", "SourceCodePro-Semibold.ttf.woff"),
    ];
    let mut ret_val = "/*(C) info FONT_LICENSES.txt*/".to_string();
    for (family, style, weight, local, url) in &fonts[..] {
        write!(&mut ret_val, "@font-face{{font-family:'{family}';font-style:{style};font-weight:{weight};src:{maybe_local}url({url})}}",
            family = family,
            style = style,
            weight = weight,
            maybe_local = (if *local != "" { format!("local('{}'),", local) } else { String::new() }),
            url = url
        ).expect("writing to string is infallible");
    }
    assert!(ret_val.len() < 1000); // the exact value is 939, BTW
    ret_val
}
lazy_static!{
    static ref FONTS_CSS: String = generate_fonts_css();
}

We could make it even smaller by renaming the fonts (.ttf.woff? really?)

@GuillaumeGomez
Copy link
Member

@notriddle That too (I didn't look at the new <links> yet). I was actually referring to the new .css file.

@jsha
Copy link
Contributor Author

jsha commented Feb 21, 2021

I pushed a new revision and demo. When I switched to <link rel="preload"> I did an incomplete job and didn't clean up files. Sorry about that! Fixed now.

font-display changes were previously tried in #72092 and #72557, then reverted in #72610

Thanks for the links to the history here, @Nemo157. I read through these. It looks like the first attempt was with font-display: optional, but that ran into issues that caused the fonts never to display. That may be related to lack of caching headers and lack of preloading, as suggested in one of the bugzilla comments linked from @workingjubilee's comment.

It looks like #72557 was later updated to use font-display: swap instead of font-display: optional, but was never merged that way. I do think font-display: swap is more stable than optional, and it matches the advice on https://web.dev/font-display/#how-to-avoid-showing-invisible-text.

To be clear about possible downsides: If font loading is slow, users may see a reflow when the fonts come in. However, the status quo is that users see a blank page until the fonts come in, and then get a reflow anyhow. And preloading and cache headers should greatly reduce the likelihood of seeing a reflow.

@notriddle
Copy link
Contributor

Personally, I would rather just inline the CSS, not use preload links. It’s not that much bigger, and it’s less total code.

@GuillaumeGomez
Copy link
Member

@notriddle Don't have much of a preference and I'm clearly not a front-end expert. What do you think about it @Nemo157 ?

@Nemo157
Copy link
Member

Nemo157 commented Feb 23, 2021

I personally err on the side of inlining more stuff, most of my personal websites target 1 request to load; a few extra bytes in the request that is going to happen anyway seem less expensive than doing an extra request.

@bors
Copy link
Contributor

bors commented Feb 26, 2021

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

@jsha
Copy link
Contributor Author

jsha commented Feb 26, 2021

#82545 means we should really do the inlined CSS version, since <link rel="preload"> doesn't know whether the browser supports woff or woff2. Let me know if that sounds good, and I'll update.

For the question of whether the inlined CSS should be a file, or should be a string in .rs file, or should be an fn that generates a string: I like the file approach since it's easier for someone to see what's in it, and edit in the plain .css format rather than going through a layer of indirection in .rs. What's the downside of making it a file?

@workingjubilee
Copy link
Contributor

workingjubilee commented Feb 27, 2021

Add font-display: swap.

IIRC font-display changes were previously tried in #72092 and #72557, then reverted in #72610, it'd be good to check whether the rationale from those still apply? (I don't personally know much about how the different settings interact, just bringing this up in case others that do have insights, maybe cc @workingjubilee).

As effectively already enumerated by @jsha: While adding font-display: optional; proved... overeager, my reasoning still held for font-display: swap; and fallback in my opinion, and I, at least, abandoned attempts to take another swing at the font-display CSS properties simply out of lack of energy to build consensus for them, not for any technical reason (nor, indeed, even a failure to write code given the state that I left #72557 in).

Add font-display: swap. Per https://web.dev/font-display/, this prevents
"flash of invisible text" during load by using a system font until the
custom font is available. I've noticed this flash of invisible text
occasionally when reading Rust docs.

Add an explicit height to icons (which already had an explicit width)
to allow browsers to lay out the page more accurately before the icons
have been loaded. https://web.dev/optimize-cls/.

Add min-width: 115px to the crate search dropdown. When the HTML first
loads, this dropdown includes only the text "All crates." Later, JS
loads the items underneath it, some of which are wider. That causes
the dropdown to get wider, causing a distracting reflow. This sets a
min-width based on the size that the dropdown eventually becomes based
on the crates on doc.rust-lang.org, reducing page movement during load.
On most platforms and browsers, `sans-serif` is equivalent to Arial.
However, on Firefox on Ubuntu (and possibly other Linuxes), `sans-serif`
is DejaVu Sans, a much wider font. This creates a larger shift in text
when the custom fonts finally load. Arial is a web-safe font, and
specifying it explicitly gives us more cross-platform consistency, as
well as reducing the layout shift that happens when fonts load.
@jsha
Copy link
Contributor Author

jsha commented Mar 3, 2021

I pushed a small update: f9cfe15 uses Arial in place of sans-serif, which is a no-op except on Firefox, where it makes things more consistent with other platforms and reduces the size of the reflow when fonts load.

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Mar 3, 2021

📌 Commit f9cfe15 has been approved by GuillaumeGomez

@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 Mar 3, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 4, 2021
…Gomez

Improve page load performance in rustdoc

Add an explicit height to icons (which already had an explicit width) to allow browsers to lay out the page more accurately before the icons have been loaded. https://web.dev/optimize-cls/.

Add min-width: 115px to the crate search dropdown. When the HTML first loads, this dropdown includes only the text "All crates." Later, JS loads the items underneath it, some of which are wider. That causes the dropdown to get wider, causing a distracting reflow. This sets a min-width based on the size that the dropdown eventually becomes based on the crates on doc.rust-lang.org, reducing page movement during load.

Add font-display: swap. Per https://web.dev/font-display/, this prevents "flash of invisible text" during load by using a system font until the custom font is available. I've noticed this flash of invisible text occasionally when reading Rust docs. Note that users without cached fonts will see text, and then see it reflow. For `docs.rust-lang.org`, [setting caching headers will help a lot](rust-lang/simpleinfra#62).

Generated output at https://jacob.hoffman-andrews.com/rust/flow-improvements/std/string/struct.String.html.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 4, 2021
…Gomez

Improve page load performance in rustdoc

Add an explicit height to icons (which already had an explicit width) to allow browsers to lay out the page more accurately before the icons have been loaded. https://web.dev/optimize-cls/.

Add min-width: 115px to the crate search dropdown. When the HTML first loads, this dropdown includes only the text "All crates." Later, JS loads the items underneath it, some of which are wider. That causes the dropdown to get wider, causing a distracting reflow. This sets a min-width based on the size that the dropdown eventually becomes based on the crates on doc.rust-lang.org, reducing page movement during load.

Add font-display: swap. Per https://web.dev/font-display/, this prevents "flash of invisible text" during load by using a system font until the custom font is available. I've noticed this flash of invisible text occasionally when reading Rust docs. Note that users without cached fonts will see text, and then see it reflow. For `docs.rust-lang.org`, [setting caching headers will help a lot](rust-lang/simpleinfra#62).

Generated output at https://jacob.hoffman-andrews.com/rust/flow-improvements/std/string/struct.String.html.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#80527 (Make rustdoc lints a tool lint instead of built-in)
 - rust-lang#82310 (Load rustdoc's JS search index on-demand.)
 - rust-lang#82315 (Improve page load performance in rustdoc)
 - rust-lang#82564 (Revert `Vec::spare_capacity_mut` impl to prevent pointers invalidation)
 - rust-lang#82697 (Fix stabilization version of move_ref_pattern)
 - rust-lang#82717 (Account for macros when suggesting adding lifetime)
 - rust-lang#82740 (Fix commit detected when using `download-rustc`)
 - rust-lang#82744 (Pass `CrateNum` by value instead of by reference)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 569f033 into rust-lang:master Mar 4, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 4, 2021
@nagisa
Copy link
Member

nagisa commented Mar 15, 2021

Note that removing the sans-serif fallback is now causing fallback to serif on systems that don't have Arial installed.

Web-safe fonts are not a thing, apparently (TIL).

@jsha
Copy link
Contributor Author

jsha commented Mar 15, 2021

Thanks for the update, @nagisa! Today I also Learned the web-safe fonts are not a thing. Want to share a link or two? Also, what systems are you finding that don't have Arial installed?

@nagisa
Copy link
Member

nagisa commented Mar 15, 2021

I've a NixOS (Linux) system, though I imagine people using any "configure yourself from scratch" kind of distribution like, say, Arch Linux would be affected too.

As far as Firefox is concerned, there's a feature where users can disallow websites to select fonts other than serif, sans-serif or monospace entirely. In which case almost any font-family rule that does not specify these exact values as a fallback will end up falling back to what Firefox calls a "proportional" – which is a serif by default, I believe.

@jsha jsha deleted the font-display-swap branch March 15, 2021 19:26
@workingjubilee workingjubilee added the A-rustdoc-ui Area: rustdoc UI (generated HTML) label Apr 9, 2021
@@ -475,7 +482,7 @@ h4 > code, h3 > code, .invisible > code {
}
#main > .since {
top: inherit;
font-family: "Fira Sans", sans-serif;
font-family: "Fira Sans", Arial;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we keep sans-serif as Arial may not always be available?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds logical indeed. Want to send a PR for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's fixed. 7134b0e

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

@jsha jsha mentioned this pull request Nov 24, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 3, 2021
rustdoc: preload fonts

Follow-up from rust-lang#82315.

I noticed that font loading was so slow that even when loading from local disk, we get a flash of unstyled text (FOUT) followed by a reflow when the fonts load. With this change, we reliably get the appropriate fonts in the first render pass when loading locally, and we get it some of the time when loading from a website.

This only preloads woff2 versions. According to https://caniuse.com/?search=preload and https://caniuse.com/?search=woff2, all browsers that support preload also support woff2, so this is fine; we will never load two copies of a font.

Don't preload italic font faces because they aren't used on all pages.

Demo: https://rustdoc.crud.net/jsha/preload-fonts/std/string/struct.String.html
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 3, 2021
rustdoc: preload fonts

Follow-up from rust-lang#82315.

I noticed that font loading was so slow that even when loading from local disk, we get a flash of unstyled text (FOUT) followed by a reflow when the fonts load. With this change, we reliably get the appropriate fonts in the first render pass when loading locally, and we get it some of the time when loading from a website.

This only preloads woff2 versions. According to https://caniuse.com/?search=preload and https://caniuse.com/?search=woff2, all browsers that support preload also support woff2, so this is fine; we will never load two copies of a font.

Don't preload italic font faces because they aren't used on all pages.

Demo: https://rustdoc.crud.net/jsha/preload-fonts/std/string/struct.String.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: rustdoc UI (generated HTML) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet