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: run css and html minifier at build instead of runtime #136253

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

notriddle
Copy link
Contributor

This way, adding a bunch of comments to the JS files won't make rustdoc slower.

Meant to address #136161 (comment)

This way, adding a bunch of comments to the JS files won't make
rustdoc slower.
@rustbot
Copy link
Collaborator

rustbot commented Jan 29, 2025

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 29, 2025
@GuillaumeGomez
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 29, 2025
@bors
Copy link
Contributor

bors commented Jan 29, 2025

⌛ Trying commit 68646e9 with merge c2ca307...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2025
rustdoc: run css and html minifier at build instead of runtime

This way, adding a bunch of comments to the JS files won't make rustdoc slower.

Meant to address rust-lang#136161 (comment)
@bors
Copy link
Contributor

bors commented Jan 29, 2025

☀️ Try build successful - checks-actions
Build commit: c2ca307 (c2ca3075e8728fb554521c1e355e4dcc332479a2)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c2ca307): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-5.4%, -0.2%] 21
Improvements ✅
(secondary)
-3.6% [-5.4%, -0.7%] 22
All ❌✅ (primary) -1.6% [-5.4%, -0.2%] 21

Max RSS (memory usage)

Results (primary 0.5%, secondary -2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [2.8%, 2.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.9% [-1.9%, -1.9%] 1
Improvements ✅
(secondary)
-2.1% [-2.3%, -1.8%] 4
All ❌✅ (primary) 0.5% [-1.9%, 2.8%] 2

Cycles

Results (primary -2.7%, secondary -3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.7% [-4.0%, -2.2%] 4
Improvements ✅
(secondary)
-3.2% [-4.0%, -1.5%] 17
All ❌✅ (primary) -2.7% [-4.0%, -2.2%] 4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 777.437s -> 777.798s (0.05%)
Artifact size: 328.50 MiB -> 328.67 MiB (0.05%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 29, 2025
@GuillaumeGomez
Copy link
Member

Seems like we're all good. :)

@lolbinarycat
Copy link
Contributor

perhaps for non-nightly builds the unminified source could be excluded entirely? although i'm not sure if that would violate some policy, since it would introduce the possibility of bugs that only show up on more stable releases.

@notriddle
Copy link
Contributor Author

I doubt that would be significant compared to all the other nightly-only code that gets included in stable rust.

@GuillaumeGomez
Copy link
Member

Depends actually, the JS is quite sizeable after all.

@notriddle
Copy link
Contributor Author

Depends actually, the JS is quite sizeable after all.

search.js is (230744 / 141483120) * 100% =~ 0.16% the size of librustc_driver.

$ wc -c src/librustdoc/html/static/js/*.js build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc build/x86_64-unknown-linux-gnu/stage2/lib/librustc_driver-d2315ca75e7f51a5.so 
    86194 src/librustdoc/html/static/js/main.js
     5350 src/librustdoc/html/static/js/scrape-examples.js
   230744 src/librustdoc/html/static/js/search.js
    11117 src/librustdoc/html/static/js/settings.js
     6160 src/librustdoc/html/static/js/src-script.js
    13611 src/librustdoc/html/static/js/storage.js
 15198168 build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc
141483120 build/x86_64-unknown-linux-gnu/stage2/lib/librustc_driver-d2315ca75e7f51a5.so
157034464 total

@GuillaumeGomez
Copy link
Member

librustc_driver is not the most common case. But the question here is more like: how much code would it need for us to be able to only add unminified code in rustdoc nightly builds?

@notriddle
Copy link
Contributor Author

notriddle commented Jan 30, 2025

librustc_driver is not the most common case.

I have no idea what you mean.

But the question here is more like: how much code would it need for us to be able to only add unminified code in rustdoc nightly builds?

The problem isn't work. It's risk.

First, because this isn't actually unreachable code in a stable build. You can use "nightly-only features" like this one with the RUSTC_BOOTSTRAP env variable. RUSTC_BOOTSTRAP=1 cargo +beta rustdoc -- -Zdisable-minification=1 probably won't get used very often, but if I had to debug a bug that appeared in stable but not in nightly, it would be handy to have.

Second, while I'm not aware of a policy against trying to strip "unreachable" code from the stable compiler, it seems pretty rare. I've tried a few major unstable features with RUSTC_BOOTSTRAP=1, and was able to use -Zpolonius and -Znext-solver=globally with a stable compiler (the polonius build was much slower than normal, so I'm pretty sure the flag wasn't just ignored).

@GuillaumeGomez
Copy link
Member

librustc_driver is not the most common case.

I have no idea what you mean.

I meant in term of doc size since it's what you displayed.

The problem isn't work. It's risk.

...

With your explanation, I agree. I forgot we could run unstable features with stable channel. Then we should provide both JS all the time.

@GuillaumeGomez
Copy link
Member

If PR is ready for you, r=me and thanks!

@notriddle
Copy link
Contributor Author

I meant in term of doc size since it's what you displayed.

The unminified JS and CSS aren't included in the emitted documentation unless you supply the -Zdisable-minification parameter. They're baked into the rustdoc binary. This change should have no effect on the size of the rust-docs.tar.gz file, or on the storage requirements for docs.rs.

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Jan 30, 2025

📌 Commit 68646e9 has been approved by GuillaumeGomez

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 Jan 30, 2025
@fmease fmease assigned GuillaumeGomez and unassigned fmease Feb 1, 2025
@fmease
Copy link
Member

fmease commented Feb 5, 2025

To place it above the next rollup @bors p=6

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2025
…illaumeGomez

rustdoc: run css and html minifier at build instead of runtime

This way, adding a bunch of comments to the JS files won't make rustdoc slower.

Meant to address rust-lang#136161 (comment)
@bors
Copy link
Contributor

bors commented Feb 5, 2025

⌛ Testing commit 68646e9 with merge 84da3b4...

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu-debug failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
drwxrwxrwt 16 root   root   4.0K Feb  5 07:25 ..
-rw-r--r--  1 runner docker 222K Feb  5 07:25 cpu-aarch64-gnu-debug.csv
-rw-r--r--  1 runner docker  74K Feb  5 07:25 metrics-aarch64-gnu-debug.json

Attempting with retry: aws s3 cp --storage-class INTELLIGENT_TIERING --no-progress --recursive --acl public-read /tmp/tmp.pNYEYwMZdU s3://rust-lang-ci2/rustc-builds/84da3b428994497dbfb892a54ae48b82ce322dbb
upload: ../../../../../tmp/tmp.pNYEYwMZdU/metrics-aarch64-gnu-debug.json to s3://rust-lang-ci2/rustc-builds/84da3b428994497dbfb892a54ae48b82ce322dbb/metrics-aarch64-gnu-debug.json
upload: ../../../../../tmp/tmp.pNYEYwMZdU/cpu-aarch64-gnu-debug.csv to s3://rust-lang-ci2/rustc-builds/84da3b428994497dbfb892a54ae48b82ce322dbb/cpu-aarch64-gnu-debug.csv
cd src/ci
npm ci
python3 scripts/upload-build-metrics.py ../../build/cpu-usage.csv
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}

@bors
Copy link
Contributor

bors commented Feb 5, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 5, 2025
@fmease
Copy link
Member

fmease commented Feb 5, 2025

spurious @bors retry

@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 Feb 5, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Feb 5, 2025

Huh, apparently the segfault I observed in rollup #136572 wasn't just one-off, it also happened here...

@bors
Copy link
Contributor

bors commented Feb 5, 2025

⌛ Testing commit 68646e9 with merge a9730c3...

@bors
Copy link
Contributor

bors commented Feb 5, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 5, 2025
@bors bors merged commit a9730c3 into rust-lang:master Feb 5, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 5, 2025
@notriddle notriddle deleted the notriddle/aot-minify branch February 5, 2025 21:22
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a9730c3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-5.5%, -0.2%] 21
Improvements ✅
(secondary)
-3.7% [-5.5%, -0.7%] 22
All ❌✅ (primary) -1.6% [-5.5%, -0.2%] 21

Max RSS (memory usage)

Results (primary 2.4%, secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.4% [1.4%, 3.4%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) 2.4% [1.4%, 3.4%] 2

Cycles

Results (primary -2.5%, secondary -3.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.5% [-4.5%, -1.7%] 7
Improvements ✅
(secondary)
-3.7% [-4.6%, -2.1%] 17
All ❌✅ (primary) -2.5% [-4.5%, -1.7%] 7

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 778.801s -> 776.778s (-0.26%)
Artifact size: 328.78 MiB -> 329.01 MiB (0.07%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

9 participants