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

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation74 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

See #84419.

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

cc @jsha @jyn514.

r? @GuillaumeGomez

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

rustbot added the T-rustdoc

Relevant to the rustdoc team, which will review and decide on the PR/issue.

label

Jan 3, 2022

djc mentioned this pull request

Jan 3, 2022

Awaiting bors try build completion.

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

⌛ Trying commit 05465c2f5081b3d866854d74745029f0bace4b63 with merge e9be93a5f7d0a32ad8a9f6a1794ee49ef250d8fa...

Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

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

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

@@ -143,8 +139,7 @@ pub(super) fn print_item(
src_href: src_href.as_deref(),
};

let teractx = tera::Context::from_serialize(item_vars).unwrap();
let heading = templates.render("print_item.html", &teractx).unwrap();
let heading = item_vars.render().unwrap();

Choose a reason for hiding this comment

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

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.

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

Contributor Author

djc commented

Jan 3, 2022

• Loading

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.

jyn514, Urgau, shepmaster, weihanglo, jsha, est31, mominul, and lnicola reacted with hooray emoji jyn514, Urgau, jsha, and mominul reacted with heart emoji

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?

Contributor Author

djc commented

Jan 3, 2022

• Loading

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

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.

@@ -7,20 +7,20 @@
{#- -#}
{#- -#}
{{page.title}} {#- -#}

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.)

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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?

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.

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)

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.

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?

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

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?

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

⌛ Testing commit ef96d57 with merge e038d9166b04292cd4f8f0ed1091ae0163dfa25b...

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

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

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

This was referenced

Jan 13, 2022

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

Urgau mentioned this pull request

Jan 13, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 14, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 14, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 14, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 19, 2022

…=notriddle

Move back templates into html folder

Follow-up of rust-lang#92526.

r? @notriddle

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.