Do more lazy-formatting in `librustdoc` 🦥 #136828

yotamofek · 2025-02-10T17:52:15Z

Modify some formatting to be lazy, i.e. to not allocate interim strings that are later formatted into different strings.
Commits are small and stand on their own, and should mostly compile separately. (The first one doesn't compile due to dead_code because all it does is introduce a helper used in later commits)

Really excited about this one, local perf results are really good. I'd love a perf run to see how this looks on CI. This is the comparison of instructions:u count between master and this PR, on my computer:

Summary

	Range	Mean	Count
Regressions	-	0.00%	0
Improvements	-8.03%, -0.40%	-2.93%	5
All	-8.03%, -0.40%	-2.93%	5

Primary benchmarks

Benchmark	Profile	Scenario	% Change	Significance Factor
typenum-1.17.0	doc	full	-8.03%	40.16x
nalgebra-0.33.0	doc	full	-4.19%	20.97x
stm32f4-0.14.0	doc	full	-1.35%	6.73x
libc-0.2.124	doc	full	-0.67%	3.33x
cranelift-codegen-0.82.1	doc	full	-0.40%	1.99x

rustbot · 2025-02-10T17:52:25Z

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
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

GuillaumeGomez · 2025-02-10T17:58:09Z

@bors try @rust-timer queue

bors · 2025-02-10T17:59:28Z

⌛ Trying commit 984a2e4 with merge a804f1e...

…r=<try> Do more lazy-formatting in `librustdoc` 🦥 Modify some formatting to be lazy, i.e. to not allocate interim strings that are later formatted into different strings. Commits are small and stand on their own, and should mostly compile separately. (The first one doesn't compile due to `dead_code` because all it does is introduce a helper used in later commits) Really excited about this one, local perf results are really good. I'd love a perf run to see how this looks on CI. This is the comparison of `instructions:u` count between master and this PR, on my computer: # Summary | | Range | Mean | Count | |:---:|:---:|:---:|:---:| | Regressions | - | 0.00% | 0 | | Improvements | -8.03%, -0.40% | -2.93% | 5 | | All | -8.03%, -0.40% | -2.93% | 5 | # Primary benchmarks | Benchmark | Profile | Scenario | % Change | Significance Factor | |:---:|:---:|:---:|:---:|:---:| | typenum-1.17.0 | doc | full | -8.03% | 40.16x | | nalgebra-0.33.0 | doc | full | -4.19% | 20.97x | | stm32f4-0.14.0 | doc | full | -1.35% | 6.73x | | libc-0.2.124 | doc | full | -0.67% | 3.33x | | cranelift-codegen-0.82.1 | doc | full | -0.40% | 1.99x |

src/librustdoc/display/maybe.rs

bors · 2025-02-10T19:52:27Z

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

yotamofek · 2025-02-10T20:57:38Z

Addressed comment in a new commit so that history is kept intact, I'll rebase if this gets approved :)

rust-timer · 2025-02-10T21:09:36Z

Finished benchmarking commit (a804f1e): 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%	[-10.2%, -0.2%]	18
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.2%]	5
All ❌✅ (primary)	-1.6%	[-10.2%, -0.2%]	18

Max RSS (memory usage)

Results (primary -3.5%, secondary -3.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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.5%	[-5.2%, -2.0%]	7
Improvements ✅ (secondary)	-3.1%	[-7.3%, -1.3%]	52
All ❌✅ (primary)	-3.5%	[-5.2%, -2.0%]	7

Cycles

Results (primary -6.8%)

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)	-6.8%	[-9.6%, -4.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-6.8%	[-9.6%, -4.0%]	2

Binary size

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

Bootstrap: 779.051s -> 779.064s (0.00%)
Artifact size: 348.29 MiB -> 348.26 MiB (-0.01%)

aDotInTheVoid

Thanks for working on this, the perf numbers look really good!

I think there's a couple of places where fmt::from_fn could be replaced with the simpler format_args!, but other than that, it all looks good.

aDotInTheVoid · 2025-02-11T00:17:21Z

src/librustdoc/html/render/mod.rs

-            if has_more_content {
-                let link = format!(" <a{}>Read more</a>", assoc_href_attr(item, link, cx));
+            let link = if has_more_content {
+                let link = fmt::from_fn(|f| {


NIT: This can be made more consise with format_args!

Doesn't work either...

aDotInTheVoid · 2025-02-11T00:20:47Z

src/librustdoc/html/render/mod.rs

    // If there is no `href` for the reason explained above, simply do not render it which is valid:
    // https://html.spec.whatwg.org/multipage/links.html#links-created-by-a-and-area-elements
-    href.map(|href| format!(" href=\"{href}\"")).unwrap_or_default()
+    Some(fmt::from_fn(move |f| write!(f, " href=\"{href}\"")))


Ditto (I think)

Doesn't work either 😅

aDotInTheVoid · 2025-02-11T00:21:43Z

src/librustdoc/html/render/mod.rs

+            write!(
+                f,
+                " <a href=\"#\" class=\"tooltip\" data-notable-ty=\"{ty}\">ⓘ</a>",
+                ty = Escape(&format!("{:#}", ty.print(cx))),


Not at all needed in this PR, but I wounder if this temporary allocation here can also go.

Yeah, I was wondering that too. I think it's unavoidable, since Escape needs to actually go over the formatted string and escape certain characters. The only way to create a "lazy" Escape construct would be to modify std's Formatter, which obviously is not an option...
But maybe I'm missing something.

aDotInTheVoid · 2025-02-11T00:22:27Z

src/librustdoc/html/render/mod.rs

-    };
+    let aliases = (!aliases.is_empty())
+        .then_some(fmt::from_fn(|f| {
+            write!(f, " data-aliases=\"{}\"", fmt::from_fn(|f| aliases.iter().joined(",", f)))


Ditto wrt format_args!

Unfortunately, doesn't work...

error[E0716]: temporary value dropped while borrowed --> src/librustdoc/html/render/mod.rs:2111:20 | 2111 | .then_some(format_args!( | ____________________- 2112 | | " data-aliases=\"{}\"", 2113 | | fmt::from_fn(|f| aliases.iter().joined(",", f)) 2114 | | )) | | ^ | | | | |_________creates a temporary value which is freed while still in use | in this macro invocation 2115 | .maybe_display(); | - temporary value is freed at the end of this statement 2116 | write!(w, "<section id=\"{id}\" class=\"impl\"{aliases}>"); | --------- borrow later used here

Ah, this is apparently a known limitation. Sorry 😔

No worries, I can never remember exactly when it can be used and when not, so I was happy to double check 😊

aDotInTheVoid · 2025-02-11T00:23:18Z

src/librustdoc/display.rs

@@ -1,3 +1,5 @@
+//! Various utilities for working with [`fmt::Display`](std::fmt::Display) implementations.


❤️ for writing a doc-comment on an internal module.

aDotInTheVoid · 2025-02-11T22:51:17Z

Addressed comment in a new commit so that history is kept intact, I'll rebase if this gets approved :)

r=me with git history cleaned up

yotamofek · 2025-02-12T06:34:22Z

@aDotInTheVoid rebased and CI is green 😁

GuillaumeGomez · 2025-02-12T09:42:46Z

Thanks!

@bors=aDotInTheVoid,GuillaumeGomez rollup=iffy

GuillaumeGomez · 2025-02-12T09:43:19Z

Sigh, way too early for me apparently...

@bors r=aDotInTheVoid,GuillaumeGomez rollup=iffy

bors · 2025-02-12T09:43:23Z

📌 Commit 96eb329 has been approved by aDotInTheVoid,GuillaumeGomez

It is now in the queue for this repository.

bors · 2025-02-13T00:19:27Z

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

GuillaumeGomez · 2025-02-13T09:54:41Z

@bors r=aDotInTheVoid,GuillaumeGomez rollup=iffy

bors · 2025-02-14T15:56:52Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Set both nuw and nsw in slice size calculation #136575

bors · 2025-02-14T15:56:53Z

📌 Commit 5cc64e8 has been approved by GuillaumeGomez,aDotInTheVoid

It is now in the queue for this repository.

…r=GuillaumeGomez,aDotInTheVoid Do more lazy-formatting in `librustdoc` 🦥 Modify some formatting to be lazy, i.e. to not allocate interim strings that are later formatted into different strings. Commits are small and stand on their own, and should mostly compile separately. (The first one doesn't compile due to `dead_code` because all it does is introduce a helper used in later commits) Really excited about this one, local perf results are really good. I'd love a perf run to see how this looks on CI. This is the comparison of `instructions:u` count between master and this PR, on my computer: # Summary | | Range | Mean | Count | |:---:|:---:|:---:|:---:| | Regressions | - | 0.00% | 0 | | Improvements | -8.03%, -0.40% | -2.93% | 5 | | All | -8.03%, -0.40% | -2.93% | 5 | # Primary benchmarks | Benchmark | Profile | Scenario | % Change | Significance Factor | |:---:|:---:|:---:|:---:|:---:| | typenum-1.17.0 | doc | full | -8.03% | 40.16x | | nalgebra-0.33.0 | doc | full | -4.19% | 20.97x | | stm32f4-0.14.0 | doc | full | -1.35% | 6.73x | | libc-0.2.124 | doc | full | -0.67% | 3.33x | | cranelift-codegen-0.82.1 | doc | full | -0.40% | 1.99x |

bors · 2025-02-14T20:19:43Z

⌛ Testing commit 5cc64e8 with merge de04531...

bors · 2025-02-14T20:21:08Z

💔 Test failed - checks-actions

rust-log-analyzer · 2025-02-14T21:03:46Z

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

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

/dev/sda15       98M  6.3M   92M   7% /boot/efi
tmpfs           1.6G  4.0K  1.6G   1% /run/user/1001
================================================================================

Failed to stop mysql.service: Unit mysql.service not loaded.
##[error]Process completed with exit code 5.

ChrisDenton · 2025-02-14T22:17:58Z

@bors retry (after #137041)

bors · 2025-02-15T11:57:18Z

⌛ Testing commit 5cc64e8 with merge 69fd5e4...

bors · 2025-02-15T14:57:39Z

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez,aDotInTheVoid
Pushing 69fd5e4 to master...

rust-timer · 2025-02-15T17:24:12Z

Finished benchmarking commit (69fd5e4): 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%	[-10.3%, -0.3%]	17
Improvements ✅ (secondary)	-0.5%	[-0.7%, -0.2%]	3
All ❌✅ (primary)	-1.6%	[-10.3%, -0.3%]	17

Max RSS (memory usage)

Results (primary 5.5%)

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)	5.5%	[5.5%, 5.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.5%	[5.5%, 5.5%]	1

Cycles

Results (primary -7.0%)

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)	-7.0%	[-8.6%, -5.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-7.0%	[-8.6%, -5.3%]	2

Binary size

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

Bootstrap: 790.255s -> 789.995s (-0.03%)
Artifact size: 347.22 MiB -> 347.21 MiB (-0.00%)

…<try> Allow lazy HTML escaping Inspired by [this comment](rust-lang#136828 (comment)) by `@aDotInTheVoid` . Makes `Escape` and `EscapeBodyText` accept any `impl fmt::Display`, instead of a `&str`, which allows us to avoid a few interim `String` allocations. This opens up room for more lazifying, but I'll keep those for a separate PR. Unfortunately, I think there might be a hit to performance because of the double vtable-indirection caused by wrapping a `fmt::Formatter` in another one, but I think that I should be able to gain that perf back by doing more lazy printing (either the small things improvements I made in this PR, or in later ones) Probably better to review each commit individually.

rustbot assigned GuillaumeGomez Feb 10, 2025

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 Feb 10, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 10, 2025

GuillaumeGomez reviewed Feb 10, 2025

View reviewed changes

src/librustdoc/display/maybe.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 10, 2025

aDotInTheVoid reviewed Feb 11, 2025

View reviewed changes

yotamofek force-pushed the pr/rustdoc/more-laziness branch from 8f77596 to 96eb329 Compare February 12, 2025 05:23

GuillaumeGomez approved these changes Feb 12, 2025

View reviewed changes

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

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 13, 2025

yotamofek force-pushed the pr/rustdoc/more-laziness branch from 96eb329 to ca1987a Compare February 13, 2025 09:28

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 14, 2025

This comment was marked as outdated.

Sign in to view

This was referenced Feb 14, 2025

Implement Extend<AsciiChar> for String #136749

Merged

llvm: Tolerate captures in tests #137038

Merged

This was referenced Feb 14, 2025

Don't remove mysql in CI #137041

Merged

add x86-sse2 (32bit) ABI that requires SSE2 target feature #137037

Merged

Rollup of 10 pull requests #137046

Merged

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 14, 2025

bors mentioned this pull request Feb 14, 2025

Remove unnecessary check code in unused_delims #137029

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 15, 2025

bors merged commit 69fd5e4 into rust-lang:master Feb 15, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Feb 15, 2025

This was referenced Feb 15, 2025

librustdoc: more usages of Joined::joined #136599

Merged

Optimize notable_trait_buttons #136816

Open

yotamofek deleted the pr/rustdoc/more-laziness branch February 15, 2025 15:55

yotamofek mentioned this pull request Feb 19, 2025

Allow lazy HTML escaping #137274

Closed

		@@ -1,3 +1,5 @@
		//! Various utilities for working with [`fmt::Display`](std::fmt::Display) implementations.

Do more lazy-formatting in librustdoc 🦥 #136828

Do more lazy-formatting in librustdoc 🦥 #136828

Conversation

yotamofek commented Feb 10, 2025

Summary

Primary benchmarks

rustbot commented Feb 10, 2025

GuillaumeGomez commented Feb 10, 2025

This comment has been minimized.

bors commented Feb 10, 2025

bors commented Feb 10, 2025

This comment has been minimized.

yotamofek commented Feb 10, 2025

rust-timer commented Feb 10, 2025

Overall result: ✅ improvements - no action needed

aDotInTheVoid left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aDotInTheVoid commented Feb 11, 2025

yotamofek commented Feb 12, 2025

GuillaumeGomez commented Feb 12, 2025

GuillaumeGomez commented Feb 12, 2025

bors commented Feb 12, 2025

bors commented Feb 13, 2025

GuillaumeGomez commented Feb 13, 2025

bors commented Feb 14, 2025

bors commented Feb 14, 2025

bors commented Feb 14, 2025

bors commented Feb 14, 2025

This comment was marked as outdated.

rust-log-analyzer commented Feb 14, 2025

ChrisDenton commented Feb 14, 2025

bors commented Feb 15, 2025

bors commented Feb 15, 2025

rust-timer commented Feb 15, 2025

Overall result: ✅ improvements - no action needed

Do more lazy-formatting in `librustdoc` 🦥 #136828

Do more lazy-formatting in `librustdoc` 🦥 #136828

aDotInTheVoid left a comment •

edited

Loading