Nuke `Buffer` abstraction from `librustdoc` 💣 #136748

yotamofek · 2025-02-08T21:06:52Z

In #136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. @GuillaumeGomez said he agrees that it can be just removed. So this PR is me removing it. So, r? @GuillaumeGomez

But...
this got a lot bigger than I had planned.
A lot of functions had a &mut Buffer arg, but instead of replacing it with a buf: impl fmt::Write arg, I decided to change those functions to return an opaque impl fmt::Display instead.

If this PR turns out to be contentious I can try to make a PR that just removes the Buffer struct and tries to make less invasive changes, but personally I do like some of the cleanups that this PR allows. Let's see what others think! I think it'll be better to review this without whitespace (If this gets positive reactions, I'll need to rebase and maybe try to separate this into logical commits, but not sure if that's very practical)

While most of the PR is "cosmetic", I did make some small changes, mostly trying to make some of the formatting lazier, and do less allocations. So a perf run will be nice :)

Pros and cons of returning `impl fmt::Display` instead of taking a `impl fmt::Write`

Cons:

Named lifetimes: function signatures got a lot more verbose because the RPIT opaque type needs to be explicitly bound by the lifetimes of the refs in the arguments
Having to use fmt::from_fn causes another level of indentation
Immutable closures, can't move out of non-Copy items (wasn't much of a problem in practice)

Pros:

Less arguments, no un-Rusty "out" argument
Nicer composability, allows the returned type to be directly used in format strings

Interchangeability

A function receiving a impl fmt::Write can be turned into a function returning a impl fmt::Display by using fmt::from_fn.

aDotInTheVoid · 2025-02-09T01:18:44Z

@bors try @rust-timer queue

bors · 2025-02-09T01:19:55Z

⌛ Trying commit be7a1ce with merge a688929...

…r=<try> Nuke `Buffer` abstraction from `librustdoc` 💣 In rust-lang#136656 I found out that the `for_html` field in the `Buffer` struct was never read, and pondered if `Buffer` had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@GuillaumeGomez` But... this got *a lot* bigger than I had planned. A lot of functions had a `&mut Buffer` arg, but instead of replacing it with a `buf: impl fmt::Write` arg, I decided to change those functions to return an opaque `impl fmt::Display` instead. If this PR turns out to be contentious I can try to make a PR that just removes the `Buffer` struct and tries to make less invasive changes, but personally I do like some of the cleanups that this PR allows. Let's see what others think! I think it'll be better to review this without whitespace (If this gets positive reactions, I'll need to rebase and maybe try to separate this into logical commits, but not sure if that's very practical) While most of the PR is "cosmetic", I did make some small changes, mostly trying to make some of the formatting lazier, and do less allocations. So a perf run will be nice :) ### Pros and cons of returning `impl fmt::Display` instead of taking a `impl fmt::Write` #### Cons: - Named lifetimes: function signatures got a lot more verbose because the RPIT opaque type needs to be explicitly bound by the lifetimes of the refs in the arguments - Having to use `fmt::from_fn` causes another level of indentation - Immutable closures, can't move out of non-`Copy` items (wasn't much of a problem in practice) #### Pros: - Less arguments, no un-Rusty "out" argument - Nicer composability, allows the returned type to be directly used in format strings ### Interchangeability A function receiving a `impl fmt::Write` can be turned into a function returning a `impl fmt::Display` by using `fmt::from_fn`.

bors · 2025-02-09T03:08:12Z

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

rust-timer · 2025-02-09T04:25:06Z

Finished benchmarking commit (a688929): comparison URL.

Overall result: ❌ regressions - 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.4%	[0.2%, 1.0%]	9
Regressions ❌ (secondary)	0.4%	[0.2%, 0.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 1.0%]	9

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 780.488s -> 781.179s (0.09%)
Artifact size: 329.04 MiB -> 329.07 MiB (0.01%)

yotamofek · 2025-02-09T09:50:56Z

Well, dammit.
Let me see if I can improve those results.
(Would still appreciate hearing a general sentiment about these changes)

aDotInTheVoid · 2025-02-09T13:24:14Z

(Would still appreciate hearing a general sentiment about these changes)

I think this PR is one big commit that does alot. I think it'd be alot easier to review and test perf and get merged if it was split up into smaller PR's. I'd start with a PR that just goes from &mut Buffer to &mut String outparams. Then do the allocation removal/moving away from outparams in several followup PR's.

yotamofek · 2025-02-09T13:34:08Z

(Would still appreciate hearing a general sentiment about these changes)

I think this PR is one big commit that does alot. I think it'd be alot easier to review and test perf and get merged if it was split up into smaller PR's. I'd start with a PR that just goes from &mut Buffer to &mut String outparams. Then do the allocation removal/moving away from outparams in several followup PR's.

Yeah, sounds like a better plan. Even if I get perf fixed, this PR would take ages to review properly.
I'll try and start with something smaller.
Thanks!

yotamofek · 2025-02-09T18:38:08Z

Superseded by #136784

…ake2, r=<try> Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748

…-take2, r=GuillaumeGomez Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748

Rollup merge of rust-lang#136784 - yotamofek:pr/rustdoc-remove-buffer-take2, r=GuillaumeGomez Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748

…play-redux, r=<try> `librustdoc`: return `impl fmt::Display` in more places instead of writing to strings Continuation of rust-lang#136784 , another attempt at landing the larger parts of rust-lang#136748 . I'd like to, gradually, make all of the building blocks for rendering docs in `librustdoc` return `impl fmt::Display` instead of returning `Strings`, or receiving a `&mut String` (or `&mut impl fmt::Write`). Another smaller end goal is to be able to get rid of [`write_str`](https://github.com/rust-lang/rust/blob/8dac72bb1d12b2649acd0c190e41524f83da5683/src/librustdoc/html/format.rs#L40-L42). This PR is a large step in that direction. Most of the changes are quite mechanical, and split up into separate commits for easier reviewing (hopefully). I took `print_item` and then started by converting all the functions it called (and their dependencies), and the last commit does the conversion for `print_item` itself. Ignoring whitespace should make reviewing a bit easier. And most importantly, perf run shows pretty good results locally, hopefully CI will also show green 😁 r? `@GuillaumeGomez` , if you feel like it.

…play-redux, r=GuillaumeGomez `librustdoc`: return `impl fmt::Display` in more places instead of writing to strings Continuation of rust-lang#136784 , another attempt at landing the larger parts of rust-lang#136748 . I'd like to, gradually, make all of the building blocks for rendering docs in `librustdoc` return `impl fmt::Display` instead of returning `Strings`, or receiving a `&mut String` (or `&mut impl fmt::Write`). Another smaller end goal is to be able to get rid of [`write_str`](https://github.com/rust-lang/rust/blob/8dac72bb1d12b2649acd0c190e41524f83da5683/src/librustdoc/html/format.rs#L40-L42). This PR is a large step in that direction. Most of the changes are quite mechanical, and split up into separate commits for easier reviewing (hopefully). I took `print_item` and then started by converting all the functions it called (and their dependencies), and the last commit does the conversion for `print_item` itself. Ignoring whitespace should make reviewing a bit easier. And most importantly, perf run shows pretty good results locally, hopefully CI will also show green 😁 r? `@GuillaumeGomez` , if you feel like it.

rustbot assigned GuillaumeGomez Feb 8, 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 8, 2025

Nuke Buffer abstraction from librustdoc 💣

be7a1ce

yotamofek force-pushed the pr/rustdoc-remove-buffer branch from 231fbd4 to be7a1ce Compare February 8, 2025 21:07

yotamofek changed the title ~~Nuke Buffer abstraction from librustdoc~~ Nuke Buffer abstraction from librustdoc 💣 Feb 8, 2025

yotamofek mentioned this pull request Feb 8, 2025

librustdoc: remove unused for_html field from Buffer 🦄 #136656

Closed

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

yotamofek mentioned this pull request Feb 9, 2025

Nuke Buffer abstraction from librustdoc, take 2 💣 #136784

Merged

yotamofek closed this Feb 9, 2025

yotamofek deleted the pr/rustdoc-remove-buffer branch February 9, 2025 18:38

yotamofek mentioned this pull request Feb 22, 2025

librustdoc: return impl fmt::Display in more places instead of writing to strings #137425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nuke `Buffer` abstraction from `librustdoc` 💣 #136748

Nuke `Buffer` abstraction from `librustdoc` 💣 #136748

yotamofek commented Feb 8, 2025 •

edited

Loading

aDotInTheVoid commented Feb 9, 2025

This comment has been minimized.

bors commented Feb 9, 2025

bors commented Feb 9, 2025

This comment has been minimized.

rust-timer commented Feb 9, 2025

yotamofek commented Feb 9, 2025 •

edited

Loading

aDotInTheVoid commented Feb 9, 2025

yotamofek commented Feb 9, 2025

yotamofek commented Feb 9, 2025

Nuke Buffer abstraction from librustdoc 💣 #136748

Nuke Buffer abstraction from librustdoc 💣 #136748

Conversation

yotamofek commented Feb 8, 2025 • edited Loading

Pros and cons of returning impl fmt::Display instead of taking a impl fmt::Write

Cons:

Pros:

Interchangeability

aDotInTheVoid commented Feb 9, 2025

This comment has been minimized.

bors commented Feb 9, 2025

bors commented Feb 9, 2025

This comment has been minimized.

rust-timer commented Feb 9, 2025

Overall result: ❌ regressions - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

yotamofek commented Feb 9, 2025 • edited Loading

aDotInTheVoid commented Feb 9, 2025

yotamofek commented Feb 9, 2025

yotamofek commented Feb 9, 2025

Nuke `Buffer` abstraction from `librustdoc` 💣 #136748

Nuke `Buffer` abstraction from `librustdoc` 💣 #136748

yotamofek commented Feb 8, 2025 •

edited

Loading

Pros and cons of returning `impl fmt::Display` instead of taking a `impl fmt::Write`

yotamofek commented Feb 9, 2025 •

edited

Loading