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

Greatly improve rustdoc rendering speed issues #56005

Merged
merged 5 commits into from Dec 15, 2018

Conversation

Projects
None yet
6 participants
@GuillaumeGomez
Member

GuillaumeGomez commented Nov 16, 2018

Fixes #55900.

So a few improvements here:

  • we're switching to DOMTokenList API when available providing a replacement if it isn't (should only happen on safari and IE I think...)
  • hide doc sections by default to allow the whole HTML generation to happen in the background to avoid triggering DOM redraw all the times (which killed the performances)

r? @QuietMisdreavus

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Nov 16, 2018

I feel like this doesn't optimize the load time on my system (Chrome, Windows 7) enough to warrant displaying practically nothing while it loads. The total loss of information while it processes is way too much of a regression for not enough of a fix. I agree with @ollie27 that a better fix would be to render less information on the page to begin with, use less items that need processing, somehow make it so that the JS doesn't need to create all these elements, something like that.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 17, 2018

The problem is mostly on the DOM manipulation, not the JS execution. I can add a bit of text but I clearly prefer to wait 3 seconds rather than having a laggy page for 30 seconds. This also fixes the issue about the fact that the docs started while being expanded when it was expected that they were collapsed.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch 2 times, most recently from c10321d to 661ef14 Nov 26, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 26, 2018

@QuietMisdreavus Okay, I was able to make it a lot faster: Going from 13 seconds to 0.5 seconds for JS execution. I guess we're good now?

@QuietMisdreavus

While i don't see the dramatic speedup you're seeing (and my Firefox's dev tools won't cooperate to let me profile the code on the Iterator page), i can at least see some speedup. This is probably fine after these comments, but i'm going to try to summon more eyes on this to make sure we fully understand the ramifications here.

Show resolved Hide resolved src/librustdoc/html/static/main.js Outdated
Show resolved Hide resolved src/librustdoc/html/static/main.js Outdated
Show resolved Hide resolved src/librustdoc/html/static/main.js Outdated
@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 2, 2018

cc @rust-lang/rustdoc to get more eyes on this PR; the transformations here all look straightforward, but i'm not too familiar with JS so i'd like to get another review pass on it.

I also want to ping @rust-lang/docs about what i think is the biggest problem with the PR. The idea is that by hiding everything to begin with, all the DOM changes we do to add fold-toggle elements won't trigger a bunch of repainting, speeding up the JS execution. However, this leads to the unfortunate consequence where generated docs require JavaScript to look at anything any more.

Based on a totally informal and non-authoritative Twitter poll, at least some people browse the internet, including Rust docs, with JS turned off, so we will be leaving some people behind here. Whether we're okay with that, i'm not willing to call by myself.

@alercah

This comment has been minimized.

Contributor

alercah commented Dec 2, 2018

I'm pretty strongly of the opinion that we should support noscript browsers or users who run with JS disabled, unless and until someone demonstrates that there is no value to it.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch from dc8d89a to d0236bd Dec 2, 2018

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Dec 2, 2018

unless and until someone demonstrates that there is no value to it.

The issue here is not that it doesn't have value. It certainly does. The issue is that if it has too much downside. We already make a number of choices, historically, that harm the average user, in order to support this use-case.

The question isn't about value, it's about balancing the pros and cons of different approaches that have different values.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch from a5387b7 to 9347839 Dec 2, 2018

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 2, 2018

After some discussion on Discord, we've added a couple new features to this:

  • Thanks to a new CSS file loaded with a <noscript> tag, we won't hide anything for people who turn JavaScript off. This solves that hangup for me, and also allows us to provide some extra help to these users in the future.
  • While the "startup" JS is running, there is a new "Loading content..." message printed in the docs:

image

This at least provides some notice to the user that the page is taking extra time to process. For the most part, this only shows up on pages with tons of content like Iterator - most pages don't have enough elements with fold toggles that need to be generated, and the initial page processing happens quickly enough that the real content gets shown before the user gets a chance to see the "loading" message.

Now that it has these changes, this PR looks good to me. Would like to see what the others (and travis) think before i r+ though.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch from 9347839 to 5d8da21 Dec 3, 2018

@bors

This comment has been minimized.

Contributor

bors commented Dec 4, 2018

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch from 5d8da21 to bd9b981 Dec 4, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 4, 2018

Rebased.

@bors

This comment has been minimized.

Contributor

bors commented Dec 6, 2018

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch from bd9b981 to d21e658 Dec 6, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 6, 2018

Rebased.

@bors

This comment has been minimized.

Contributor

bors commented Dec 6, 2018

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

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:speedup-doc-render branch from d21e658 to b8c1726 Dec 6, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 8, 2018

@QuietMisdreavus travis is happy and we are getting new issues about this. Do you want to wait a bit longer or do you think it's ready to go?

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 8, 2018

cc @rust-lang/rustdoc @rust-lang/docs to take another look. Does the new support of NoScript users and the new "loading" message make this PR mergeable?

@@ -23,6 +23,9 @@ pub static RUSTDOC_CSS: &'static str = include_str!("static/rustdoc.css");
/// The file contents of `settings.css`, responsible for the items on the settings page.
pub static SETTINGS_CSS: &'static str = include_str!("static/settings.css");

/// The file contents of the `noscript.css` file, used in case JS isn't supported or is disabled.
pub static NOSCRIPT_CSS: &'static str = include_str!("static/noscript.css");

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Dec 10, 2018

Member

Just realized: New static file, need to update docs.rs when this is merged.

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 14, 2018

Given the rousing lack of comment, i'm going to go ahead and push this one out.

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Dec 14, 2018

📌 Commit b8c1726 has been approved by QuietMisdreavus

@bors

This comment has been minimized.

Contributor

bors commented Dec 15, 2018

⌛️ Testing commit b8c1726 with merge 56b30e2...

bors added a commit that referenced this pull request Dec 15, 2018

Auto merge of #56005 - GuillaumeGomez:speedup-doc-render, r=QuietMisd…
…reavus

Greatly improve rustdoc rendering speed issues

Fixes #55900.

So a few improvements here:

* we're switching to `DOMTokenList` API when available providing a replacement if it isn't (should only happen on safari and IE I think...)
* hide doc sections by default to allow the whole HTML generation to happen in the background to avoid triggering DOM redraw all the times (which killed the performances)

r? @QuietMisdreavus
@bors

This comment has been minimized.

Contributor

bors commented Dec 15, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 15, 2018

The job dist-aarch64-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:03:48] travis_fold:start:stage2-cargo
travis_time:start:stage2-cargo
Building stage2 tool cargo (aarch64-unknown-linux-gnu)
[01:03:48]  Downloading crates ...
[01:03:59] error: failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`
[01:03:59] Caused by:
[01:03:59]   [35] SSL connect error (OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to static.crates.io:443 )
[01:03:59] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "aarch64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json"
[01:03:59] expected success, got: exit code: 101
---
travis_time:end:126e4c88:start=1544847764213454945,finish=1544847764226934085,duration=13479140
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02eecc97
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:020f1e24
travis_time:start:020f1e24
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:006ec426
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Dec 15, 2018


[01:03:48] �[0m�[0m�[1m�[32m Downloading�[0m crates ...

[01:03:59] �[0m�[0m�[1m�[31merror:�[0m failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`

[01:03:59] 

[01:03:59] Caused by:

[01:03:59]   [35] SSL connect error (OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to static.crates.io:443 

@bors retry

@bors

This comment has been minimized.

Contributor

bors commented Dec 15, 2018

⌛️ Testing commit b8c1726 with merge 7f04a64...

bors added a commit that referenced this pull request Dec 15, 2018

Auto merge of #56005 - GuillaumeGomez:speedup-doc-render, r=QuietMisd…
…reavus

Greatly improve rustdoc rendering speed issues

Fixes #55900.

So a few improvements here:

* we're switching to `DOMTokenList` API when available providing a replacement if it isn't (should only happen on safari and IE I think...)
* hide doc sections by default to allow the whole HTML generation to happen in the background to avoid triggering DOM redraw all the times (which killed the performances)

r? @QuietMisdreavus
@bors

This comment has been minimized.

Contributor

bors commented Dec 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 7f04a64 to master...

@bors bors merged commit b8c1726 into rust-lang:master Dec 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:speedup-doc-render branch Dec 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment