Migrate rustdoc from Tera to Askama by djc · Pull Request #92526 · rust-lang/rust

djc · 2022-01-03T17:21:28Z

See #84419.

Should probably get a benchmarking run to verify if it has the intended effect on rustdoc performance.

cc @jsha @jyn514.

rust-highfive · 2022-01-03T17:21:31Z

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

GuillaumeGomez · 2022-01-03T18:12:15Z

Let's check perf.

@bors try @rust-timer queue

rust-timer · 2022-01-03T18:12:17Z

Awaiting bors try build completion.

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

bors · 2022-01-03T18:12:25Z

⌛ Trying commit 05465c2f5081b3d866854d74745029f0bace4b63 with merge e9be93a5f7d0a32ad8a9f6a1794ee49ef250d8fa...

jsha

Thanks for working on this! Overall looks great. A few questions below.

bors · 2022-01-03T19:33:44Z

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

rust-timer · 2022-01-03T19:33:45Z

Queued e9be93a5f7d0a32ad8a9f6a1794ee49ef250d8fa with parent ddabe07, future comparison URL.

djc · 2022-01-03T20:29:12Z

A further performance improvement here would be to make sure that Buffer impls core::fmt::Write, in which case we could use render_into(buf) rather than render(), avoiding the allocation of a temporary String here. Buffer internally seems to just hold a String and it has write_str() and write_fmt() methods already, so perhaps it would make sense to implement that trait? I'm not sure how big/relevant the performance improvement would be.

rust-timer · 2022-01-03T21:04:45Z

Finished benchmarking commit (e9be93a5f7d0a32ad8a9f6a1794ee49ef250d8fa): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

Very large improvement in instruction counts (up to -30.8% on full builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

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

djc · 2022-01-03T21:24:27Z

Looks like this easily wins back performance from #89695 and thus fixes #89732:

Benchmark & Profile	Scenario	% Change	Significance factor	Before #89695	After #89695	This PR
externs	full	-30.82%	210.60x	4189786474.00	4344364681.00	2946465638.00
stm32f4	full	-12.20%?	39.40x	56031165508.00	57091116514.00	45580042347.00
style-servo	full	-9.81%	83.44x	59526418329.00	60422392129.00	47964593547.00
html5ever	full	-3.94%	20.06x	5015123198.00	5043144386.00	4373075414.00

It looks like it's even substantially better than the situation before using Tera.

GuillaumeGomez · 2022-01-03T22:00:45Z

So performance is great, code changes look good. Can you confirm that the generated HTML files size doesn't change (before and after this PR) please?

djc · 2022-01-03T22:16:42Z

Can you recommend an easy way/entry point on how/where to check that?

GuillaumeGomez · 2022-01-03T22:23:17Z

Sure! So first, use x.py doc std --stage 1 to generate docs in build/[your archi]/doc. Then use du -s build/[your archi]/doc. Remove this folder once done and switch to your branch and repeat both commands.

camelid · 2022-01-03T23:59:47Z

Is there a way to get Askama to automatically strip the extra space rather than having to use {#- -#} all over the place? This is one of my annoyances with templates, and it seems like it should theoretically be solvable.

There is not currently. Askama is a general-purpose text templating mechanism, and the "extra space" is only useless in the particular context of writing out HTML. I don't know the exact details of how reducing whitespace in a string serialization of the HTML DOM will affect that DOM, but if there is a reliable algorithm to implement that, I'd suggest you open an issue against Askama to discuss how we might support that.
(For example, one way I might imagine doing it is by stripping any whitespace after a newline, but that doesn't remove all the whitespace the {#- -#} currently remove because it leaves the newlines themselves. However, if you strip the newlines themselves you would invalidate content like <img src="foo"\nalt="foo">. So solving this optimally seems like a decently hard problem.)

At least stripping the extra spaces after the newline seems like it'd be good enough to me. Sometimes the newlines can be helpful for debugging the HTML too.

I don't think I will have time to write code for this, but I'm happy to discuss how it could be implemented and review the code.

One of the Askama team members submitted a PR for something like this today: askama-rs/askama-old#598.

camelid · 2022-01-04T05:07:52Z

What do the errors look like when a template parameter is used incorrectly? E.g., what's the error when you try to iterate over an integer in a template?

djc · 2022-01-04T09:46:46Z

What do the errors look like when a template parameter is used incorrectly? E.g., what's the error when you try to iterate over an integer in a template?

The errors are somewhat painful: you get a normal compiler error but with the span pointing to the derive(Template). As far as I know there is no straightforward way to fix this because (a) procedural macros cannot check types themselves and (b) procedural macros cannot create new spans that point outside the Rust source code. While creating this PR I resorted to using cargo expand to expand the template, copying over the generated code to get a better error, then fixing it up in the template. Ideally you'd write smaller bits of template at a time so that you know roughly where the error would be coming from.

djc · 2022-01-04T12:30:12Z

Sure! So first, use x.py doc std --stage 1 to generate docs in build/[your archi]/doc. Then use du -s build/[your archi]/doc. Remove this folder once done and switch to your branch and repeat both commands.

Building the std docs this way errors out for me:

 Documenting core v0.0.0 (/Users/djc/src/rust/library/core)
dyld[64157]: Library not loaded: @rpath/libtest-88c87912f839b84d.dylib
Referenced from: /Users/djc/src/rust/build/aarch64-apple-darwin/stage1/bin/rustdoc
Reason: tried: '/Users/djc/src/rust/build/aarch64-apple-darwin/stage1/lib/libtest-88c87912f839b84d.dylib' (no such file), '/Users/djc/src/rust/build/aarch64-apple-darwin/stage1/bin/../lib/libtest-88c87912f839b84d.dylib' (no such file), '/Users/djc/src/rust/build/aarch64-apple-darwin/stage1/bin/../lib/libtest-88c87912f839b84d.dylib' (no such file), '/Users/djc/src/rust/build/aarch64-apple-darwin/stage1-std/release/deps/libtest-88c87912f839b84d.dylib' (no such file), '/Users/djc/src/rust/build/aarch64-apple-darwin/stage0/lib/libtest-88c87912f839b84d.dylib' (no such file), '/Users/djc/lib/libtest-88c87912f839b84d.dylib' (no such file), '/usr/local/lib/libtest-88c87912f839b84d.dylib' (no such file), '/usr/lib/libtest-88c87912f839b84d.dylib' (no such file)

GuillaumeGomez · 2022-01-04T12:46:51Z

Ah right, sometimes for whatever reason it just doesn't build. Use x.py clean beforehand and try again... If it still doesn't work, remove everything in the build folder and try again.

I was sure that this issue was fixed though... So weird.

jyn514 · 2022-01-04T13:57:12Z

Documenting core v0.0.0 (/Users/djc/src/rust/library/core)
dyld[64157]: Library not loaded: @rpath/libtest-88c87912f839b84d.dylib
Referenced from: /Users/djc/src/rust/build/aarch64-apple-darwin/stage1/bin/rustdoc
Reason: tried: '/Users/djc/src/rust/build/aarch64-apple-darwin/stage1/lib/libtest-88c87912f839b84d.dylib' (no such file)

That looks like you haven't built the standard library yet, which is strange since I thought x.py should build it before building rustdoc ... does x.py build --stage 1 std && x.py doc --stage 1 std help?

GuillaumeGomez · 2022-01-04T14:01:37Z

It's in the build requirements normally as you can see here.

camelid · 2022-01-04T20:45:15Z

What do the errors look like when a template parameter is used incorrectly? E.g., what's the error when you try to iterate over an integer in a template?
The errors are somewhat painful: you get a normal compiler error but with the span pointing to the derive(Template). As far as I know there is no straightforward way to fix this because (a) procedural macros cannot check types themselves and (b) procedural macros cannot create new spans that point outside the Rust source code. While creating this PR I resorted to using cargo expand to expand the template, copying over the generated code to get a better error, then fixing it up in the template. Ideally you'd write smaller bits of template at a time so that you know roughly where the error would be coming from.

Hmm... I wonder if there's a way you could attach the spans to non-Rust code by doing something like

const _:&str = include_str!("the_template.html");

Then, the template source code would be part of the SourceMap, but it would (should) be optimized out of the final binary. I'm not sure how you'd get access to those spans from Askama, but maybe it's worth looking into?

camelid · 2022-01-04T20:46:24Z

Why did you move the templates from src/librustdoc/html/templates to src/librustdoc/templates? The old location seems better to me.

bors · 2022-01-12T13:46:28Z

⌛ Testing commit ef96d57 with merge e038d9166b04292cd4f8f0ed1091ae0163dfa25b...

rust-log-analyzer · 2022-01-12T14:35:48Z

A job failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2022-01-12T14:36:17Z

💔 Test failed - checks-actions

GuillaumeGomez · 2022-01-12T14:38:57Z

@bors retry

matthiaskrgr · 2022-01-13T00:07:49Z

@bors p=11 prioritizing over #92826

bors · 2022-01-13T00:29:37Z

⌛ Testing commit ef96d57 with merge e916815...

bors · 2022-01-13T03:21:35Z

☀️ Test successful - checks-actions
Approved by: jsha
Pushing e916815 to master...

rust-timer · 2022-01-13T04:52:02Z

Finished benchmarking commit (e916815): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

Very large improvement in instruction counts (up to -30.7% on full builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Fix a missing dot in the main item heading This pull-request fix a missing `·` in the item header ~~and also make use of ` ` to explicit that the spaces are mandatory~~. | Before | After | | --- | --- | | ![image](https://user-images.githubusercontent.com/3616612/149393966-7cca6dc5-9a62-47fa-8c9c-18f936d43aa9.png) | ![image](https://user-images.githubusercontent.com/3616612/149393869-5ffd6e44-d91c-4ece-b69e-d103304f6626.png) | PS: This was introduce yesterday by rust-lang#92526 (the migration from Tera to Askama) and is not currently observable in the nightly doc.

…=notriddle Move back templates into html folder Follow-up of rust-lang#92526. r? `@notriddle`

rust-highfive assigned GuillaumeGomez Jan 3, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 3, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2022

djc mentioned this pull request Jan 3, 2022
rustdoc: use a templating engine to generate HTML #84419
Closed

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 3, 2022

weihanglo reviewed Jan 3, 2022
View reviewed changes

Comment threadsrc/librustdoc/html/layout.rs Outdated

jsha reviewed Jan 3, 2022
View reviewed changes

Comment threadsrc/librustdoc/templates/page.html Outdated
Comment threadsrc/librustdoc/templates/page.html Outdated

djc commented Jan 3, 2022
View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 3, 2022

camelid reviewed Jan 3, 2022
View reviewed changes

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 Jan 12, 2022

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 12, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2022

bors merged commit e916815 into rust-lang:masterJan 13, 2022

rustbot added this to the 1.60.0 milestone Jan 13, 2022

Urgau mentioned this pull request Jan 13, 2022
Fix a missing dot in the main item heading #92864
Merged

GuillaumeGomez mentioned this pull request Jan 17, 2022
Move back templates into html folder #93005
Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 19, 2022
Rollup merge ofrust-lang#93005- GuillaumeGomez:templates-in-html, r…
623791d
…=notriddle Move back templates into html folder Follow-up of rust-lang#92526. r? `@notriddle`

GuillaumeGomez mentioned this pull request Oct 27, 2022
Fix performance regression from #89695 #89732
Closed

jyn514 mentioned this pull request Mar 7, 2023
rustdoc: Tracking issue for the Askama migration #108868
Open
25 tasks

Uh oh!

Uh oh!

Conversation

djc commented Jan 3, 2022

Uh oh!

rust-highfive commented Jan 3, 2022

Uh oh!

GuillaumeGomez commented Jan 3, 2022

Uh oh!

rust-timer commented Jan 3, 2022

Uh oh!

bors commented Jan 3, 2022

Uh oh!

Uh oh!

jsha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Jan 3, 2022

Uh oh!

rust-timer commented Jan 3, 2022

Uh oh!

djcJan 3, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-timer commented Jan 3, 2022

Uh oh!

djc commented Jan 3, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Jan 3, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc commented Jan 3, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Jan 3, 2022

Uh oh!

camelidJan 3, 2022

Choose a reason for hiding this comment

Uh oh!

djcJan 4, 2022

Choose a reason for hiding this comment

Uh oh!

camelidJan 4, 2022

Choose a reason for hiding this comment

Uh oh!

djcJan 4, 2022

Choose a reason for hiding this comment

Uh oh!

djcJan 6, 2022

Choose a reason for hiding this comment

Uh oh!

camelid commented Jan 4, 2022

Uh oh!

djc commented Jan 4, 2022

Uh oh!

djc commented Jan 4, 2022

Uh oh!

GuillaumeGomez commented Jan 4, 2022

Uh oh!

jyn514 commented Jan 4, 2022

Uh oh!

GuillaumeGomez commented Jan 4, 2022

Uh oh!

camelid commented Jan 4, 2022

Uh oh!

camelid commented Jan 4, 2022

Uh oh!

bors commented Jan 12, 2022

Uh oh!

rust-log-analyzer commented Jan 12, 2022

Uh oh!

bors commented Jan 12, 2022

Uh oh!

GuillaumeGomez commented Jan 12, 2022

Uh oh!

matthiaskrgr commented Jan 13, 2022

djcJan 3, 2022•
edited
Loading

djc commented Jan 3, 2022•
edited
Loading

GuillaumeGomez commented Jan 3, 2022•
edited
Loading

djc commented Jan 3, 2022•
edited
Loading