Refactor `fmt::Display` impls in rustdoc #135494

yotamofek · 2025-01-14T17:18:06Z

This PR does a couple of things, with the intention of cleaning up and streamlining some of the fmt::Display impls in rustdoc:

Use the unstable fmt::from_fn instead of open-coding it.
~~Replace bespoke implementations of Itertools::format with the method itself.~~
Some more minor cleanups - DRY, remove unnecessary calls to Symbol::as_str(), replace some format!() calls with lazier options

The changes are mostly cosmetic but some of them might have a slight positive effect on performance.

rustbot · 2025-01-14T17:18:15Z

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

yotamofek · 2025-01-14T17:19:32Z

src/librustdoc/clean/cfg.rs

@@ -389,98 +390,84 @@ fn write_with_opt_paren<T: fmt::Display>(
    Ok(())
 }

+impl Display<'_> {
+    fn display_quantified_sub_cfgs(


I really don't think this is a great name for this function, but couldn't come up with a better one 😅

I would just go for display_sub_cfgs since the only constructs (of the cfg sublanguage) that may have sub-cfgs are all and any.

I guess “quantified” is not wrong since it's true that the sub cfgs are quantified by either $\forall$ or $\exists$ over a finite domain $\mathbb{F}_n$ — aka n-ary conjunction ($\bigwedge_{i=0}^n$) and n-ary disjunction ($\bigvee_{i=0}^n$), respectively. Anyway ^^

Renamed. I just looked up "common term for ALL and ANY" or something haha. Math is not my strong suit, so thanks for the thorough explanation, I appreciate it 😁

I was considering making a Quantifier enum with Any or All variants to make this function a bit easier to understand, but not sure if it's not too verbose. Leaving at is as is for now, unless you think it's a good idea.

aDotInTheVoid · 2025-01-20T22:54:59Z

@bors try @rust-timer queue

Refactor `fmt::Display` impls in rustdoc This PR does a couple of things, with the intention of cleaning up and streamlining some of the `fmt::Display` impls in rustdoc: 1. Use the unstable [`fmt::from_fn`](rust-lang#117729) instead of open-coding it. 2. Replace bespoke implementations of `Itertools::format` with the method itself. 3. Some more minor cleanups - DRY, remove unnecessary calls to `Symbol::as_str()`, replace some `format!()` calls with lazier options The changes are mostly cosmetic but some of them might have a slight positive effect on performance.

bors · 2025-01-20T22:56:12Z

⌛ Trying commit 16e9fe6 with merge 6b19da6...

bors · 2025-01-21T00:45:00Z

☀️ Try build successful - checks-actions
Build commit: 6b19da6 (6b19da6ca4f7739aa3366e3d606c076de97524c3)

rust-timer · 2025-01-21T02:04:47Z

Finished benchmarking commit (6b19da6): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.9%	[0.4%, 1.5%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[-1.0%, 1.5%]	5

Max RSS (memory usage)

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

Cycles

Results (secondary 9.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)	-	-	0
Regressions ❌ (secondary)	9.9%	[9.4%, 10.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 765.975s -> 765.445s (-0.07%)
Artifact size: 326.02 MiB -> 325.98 MiB (-0.01%)

yotamofek · 2025-01-21T18:31:20Z

@aDotInTheVoid should I do something about the perf results? AFAIR rustc-perf doesn't actually benchmark rustdoc, so I would guess these changes must be noise?

fmease · 2025-01-21T19:07:56Z

@yotamofek It does benchmark rustdoc. There are 4 profile=doc (aka rustdoc) regressions, the check one is spurious.

yotamofek · 2025-01-21T19:18:33Z

Oh, ok, I see it now. So it's definitely a real regression.
That sucks, I think using Itertools::format is much nicer than re-implementing it so many times, but it uses Cell under the hood (because Display takes an immutable ref to self) so that might be the source of the regression.
Will look into it.
Thanks!

yotamofek · 2025-01-21T20:10:50Z

I removed the commit with the usages of Itertools::format, would love another perf run to eliminate the other commits from being the culprits for the regression. @aDotInTheVoid or @fmease

aDotInTheVoid · 2025-01-21T20:52:49Z

@bors try @rust-timer queue

Refactor `fmt::Display` impls in rustdoc This PR does a couple of things, with the intention of cleaning up and streamlining some of the `fmt::Display` impls in rustdoc: 1. Use the unstable [`fmt::from_fn`](rust-lang#117729) instead of open-coding it. 2. Replace bespoke implementations of `Itertools::format` with the method itself. 3. Some more minor cleanups - DRY, remove unnecessary calls to `Symbol::as_str()`, replace some `format!()` calls with lazier options The changes are mostly cosmetic but some of them might have a slight positive effect on performance.

bors · 2025-01-21T20:54:02Z

⌛ Trying commit 37c30a0 with merge 109d05c...

bors · 2025-01-21T22:41:54Z

☀️ Try build successful - checks-actions
Build commit: 109d05c (109d05ce649ea6dde19695dbd197cbf63bb11520)

fmease · 2025-01-22T01:01:21Z

@rust-timer ping

fmease · 2025-01-22T01:02:34Z

@rust-timer build 109d05c

rust-timer · 2025-01-22T02:19:18Z

Finished benchmarking commit (109d05c): 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)	-0.2%	[-0.2%, -0.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	2

Max RSS (memory usage)

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

Cycles

Results (secondary 2.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)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 764.328s -> 765.066s (0.10%)
Artifact size: 326.09 MiB -> 326.05 MiB (-0.01%)

yotamofek · 2025-01-22T05:23:02Z

The first commit likely doesn't have an effect on perf / runtime behavior because the compiler should be smart enough to recognize that it's better to pass the large parameter by reference. However, it's still nicer to make it explicit.

Actually, that was needed to make it easier to adopt fmt::from_fn (which takes a Fn and not an FnOnce, like display_fn did, so requires Copy-able captures for its closures)

fmease · 2025-01-22T05:45:52Z

The first commit likely doesn't have an effect on perf / runtime behavior because the compiler should be smart enough to recognize that it's better to pass the large parameter by reference. However, it's still nicer to make it explicit.

Actually, that was needed to make it easier to adopt fmt::from_fn (which takes a Fn and not an FnOnce, like display_fn did, so requires Copy-able captures for its closures)

Ah, I see. Thanks for the correction, that makes perfect sense!

@bors r+

bors · 2025-01-22T05:45:54Z

📌 Commit 48b5481 has been approved by fmease

It is now in the queue for this repository.

bors · 2025-01-23T09:30:52Z

⌛ Testing commit 48b5481 with merge fc0094f...

bors · 2025-01-23T12:37:53Z

☀️ Test successful - checks-actions
Approved by: fmease
Pushing fc0094f to master...

rust-timer · 2025-01-23T13:56:08Z

Finished benchmarking commit (fc0094f): 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)	-0.2%	[-0.2%, -0.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	2

Max RSS (memory usage)

Results (secondary 1.6%)

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)	1.6%	[0.5%, 3.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -2.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.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

Binary size

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

Bootstrap: 776.757s -> 778.767s (0.26%)
Artifact size: 325.99 MiB -> 325.93 MiB (-0.02%)

…rrowLii Use `fmt::from_fn` in more places in the compiler Use the unstable functions from rust-lang#117729 in more places in the compiler, follow up to rust-lang#135494

Rollup merge of rust-lang#135951 - yotamofek:use-debug-helpers, r=SparrowLii Use `fmt::from_fn` in more places in the compiler Use the unstable functions from rust-lang#117729 in more places in the compiler, follow up to rust-lang#135494

[WIP: perf] attempt to prevent regressions in that originally happened in rust-lang#135494 This implements something similar to [`Itertools::format`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format), but on `Fn`s returning iterators instead of directly on iterators, to allow implementing `Display` without the use of a `Cell` (to handle the possibility of `fmt` being called multiple times while receiving `&self`). This is WIP, I just want to get a perf run first to see if the regression I saw in rust-lang#135494 is fixed

… r=dtolnay Mark `std::fmt::from_fn` as `#[must_use]` While working on rust-lang#135494 I managed to shoot my own foot a few times by forgetting to actually use the result of `fmt::from_fn`, so I think a `#[must_use]` could be appropriate! Didn't have a good message to put in the attr so left it blank, still unstable so we can come back to it I guess? cc rust-lang#117729 (and a huge +1 for getting it stabilized, it's very useful IMHO)

…illaumeGomez librustdoc: create a helper for separating elements of an iterator instead of implementing it multiple times This implements something similar to [`Itertools::format`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format), but on `Fn`s returning iterators instead of directly on iterators, to allow implementing `Display` without the use of a `Cell` (to handle the possibility of `fmt` being called multiple times while receiving `&self`). ~This is WIP, I just want to get a perf run first to see if the regression I saw in rust-lang#135494 is fixed~ This was originally part of rust-lang#135494 , but originally caused a perf regression that was since fixed: https://github.com/rust-lang/rust/blob/7d5ae1863aa66847a4edf8d2ef9420717df65c5d/src/librustdoc/html/format.rs#L507

… r=dtolnay Mark `std::fmt::from_fn` as `#[must_use]` While working on rust-lang#135494 I managed to shoot my own foot a few times by forgetting to actually use the result of `fmt::from_fn`, so I think a `#[must_use]` could be appropriate! Didn't have a good message to put in the attr so left it blank, still unstable so we can come back to it I guess? cc rust-lang#117729 (and a huge +1 for getting it stabilized, it's very useful IMHO)

Rollup merge of rust-lang#136502 - yotamofek:pr/fmt-from-fn-must-use, r=dtolnay Mark `std::fmt::from_fn` as `#[must_use]` While working on rust-lang#135494 I managed to shoot my own foot a few times by forgetting to actually use the result of `fmt::from_fn`, so I think a `#[must_use]` could be appropriate! Didn't have a good message to put in the attr so left it blank, still unstable so we can come back to it I guess? cc rust-lang#117729 (and a huge +1 for getting it stabilized, it's very useful IMHO)

rustbot assigned fmease Jan 14, 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 Jan 14, 2025

yotamofek changed the title ~~Rustdoc fmt from fn~~ Refactor fmt::Display impls in rustdoc Jan 14, 2025

yotamofek commented Jan 14, 2025

View reviewed changes

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 Jan 20, 2025

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 21, 2025

yotamofek force-pushed the rustdoc-fmt-from_fn branch from 16e9fe6 to 37c30a0 Compare January 21, 2025 20:09

This comment has been minimized.

Sign in to view

rustbot removed the perf-regression Performance regression. label Jan 22, 2025

yotamofek added 2 commits January 22, 2025 05:13

rustdoc: pass around decoration info by ref

73fc7af

rustdoc: use std's (unstable) fmt::from_fn instead of open-coding it

1dee842

yotamofek force-pushed the rustdoc-fmt-from_fn branch from 37c30a0 to 57de8b7 Compare January 22, 2025 05:16

This comment has been minimized.

Sign in to view

rustdoc: extract duplicated code into method

48b5481

yotamofek force-pushed the rustdoc-fmt-from_fn branch from 57de8b7 to 48b5481 Compare January 22, 2025 05:31

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 23, 2025

bors merged commit fc0094f into rust-lang:master Jan 23, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 23, 2025

yotamofek deleted the rustdoc-fmt-from_fn branch January 23, 2025 12:38

yotamofek mentioned this pull request Jan 24, 2025

Use fmt::from_fn in more places in the compiler #135951

Merged

yotamofek mentioned this pull request Jan 29, 2025

librustdoc: create a helper for separating elements of an iterator instead of implementing it multiple times #136244

Merged

yotamofek mentioned this pull request Feb 3, 2025

Mark std::fmt::from_fn as #[must_use] #136502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `fmt::Display` impls in rustdoc #135494

Refactor `fmt::Display` impls in rustdoc #135494

yotamofek commented Jan 14, 2025 •

edited

Loading

rustbot commented Jan 14, 2025

yotamofek Jan 14, 2025

fmease Jan 22, 2025 •

edited

Loading

yotamofek Jan 22, 2025

yotamofek Jan 22, 2025

aDotInTheVoid commented Jan 20, 2025

This comment has been minimized.

bors commented Jan 20, 2025

bors commented Jan 21, 2025

This comment has been minimized.

rust-timer commented Jan 21, 2025

yotamofek commented Jan 21, 2025

fmease commented Jan 21, 2025

yotamofek commented Jan 21, 2025

yotamofek commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025

bors commented Jan 21, 2025

bors commented Jan 21, 2025

fmease commented Jan 22, 2025

fmease commented Jan 22, 2025

This comment has been minimized.

rust-timer commented Jan 22, 2025

yotamofek commented Jan 22, 2025 •

edited

Loading

This comment has been minimized.

fmease commented Jan 22, 2025

bors commented Jan 22, 2025

bors commented Jan 23, 2025

bors commented Jan 23, 2025

rust-timer commented Jan 23, 2025

Refactor fmt::Display impls in rustdoc #135494

Refactor fmt::Display impls in rustdoc #135494

Conversation

yotamofek commented Jan 14, 2025 • edited Loading

rustbot commented Jan 14, 2025

Choose a reason for hiding this comment

fmease Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aDotInTheVoid commented Jan 20, 2025

This comment has been minimized.

bors commented Jan 20, 2025

bors commented Jan 21, 2025

This comment has been minimized.

rust-timer commented Jan 21, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

yotamofek commented Jan 21, 2025

fmease commented Jan 21, 2025

yotamofek commented Jan 21, 2025

yotamofek commented Jan 21, 2025

aDotInTheVoid commented Jan 21, 2025

bors commented Jan 21, 2025

bors commented Jan 21, 2025

fmease commented Jan 22, 2025

fmease commented Jan 22, 2025

This comment has been minimized.

rust-timer commented Jan 22, 2025

Overall result: ✅ improvements - no action needed

yotamofek commented Jan 22, 2025 • edited Loading

This comment has been minimized.

fmease commented Jan 22, 2025

bors commented Jan 22, 2025

bors commented Jan 23, 2025

bors commented Jan 23, 2025

rust-timer commented Jan 23, 2025

Overall result: ✅ improvements - no action needed

Refactor `fmt::Display` impls in rustdoc #135494

Refactor `fmt::Display` impls in rustdoc #135494

yotamofek commented Jan 14, 2025 •

edited

Loading

fmease Jan 22, 2025 •

edited

Loading

yotamofek commented Jan 22, 2025 •

edited

Loading