Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 #136784

yotamofek · 2025-02-09T18:37:25Z

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? @aDotInTheVoid , maybe?

Supersedes #136748

aDotInTheVoid · 2025-02-09T19:17:45Z

@bors try @rust-timer queue

…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

bors · 2025-02-09T19:18:56Z

⌛ Trying commit cbfbc2f with merge 3254d61...

bors · 2025-02-09T21:09:38Z

☀️ Try build successful - checks-actions
Build commit: 3254d61 (3254d61826419cbae5637a841f3741a379497295)

rust-timer · 2025-02-09T23:21:21Z

Finished benchmarking commit (3254d61): 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
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Max RSS (memory usage)

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

Cycles

Results (primary -2.3%)

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

Binary size

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

Bootstrap: 779.273s -> 779.002s (-0.03%)
Artifact size: 329.09 MiB -> 329.20 MiB (0.03%)

yotamofek · 2025-02-10T07:14:14Z

Both improvements and regressions seem unrelated (not on the "Doc" profile), so this is probably neutral, which makes sense.

GuillaumeGomez · 2025-02-10T19:49:09Z

src/librustdoc/html/format.rs

-}
+/// This macro is the same as [`std::write!`] for [`String`]s, but swallows the returned `Result`,
+/// since writing into a `String` can never fail.
+macro_rules! write_str {


Please avoid adding macros to do such little operations. You can instead write a function expecting a String so that it doesn't need a new trait for each write_str!/write! call. Also, renaming the macro into write makes the code reading a lot more complex.

The reason why this is a macro (and why write! itself is a macro in the STD), is that otherwise every call would like something like this:

let mut buf = String::new(); write_str(&mut buf, format_args!("format str, {}", var)); // instead of: write!(buf, "format str, {}", var);

I don't like this macro either, but I think the best alternative would be to add let _ = ... or unwrap()s to every call to write! on a string (which would be very verbose), or leave the Buffer be.

For the record, my future plan is to move as many functions as possible to use RPIT to return an impl fmt::Display, instead of accepting a &mut String as a param, and then get rid of this macro. I tried to do that in #136748 , but that PR became very big...

Still better than writing a new macro, using format_args is fine. As a first step until something better comes up is ok.

Done.
Not sure I love the function names (write_str and writeln_str), if you have a better idea in mind I'll be happy to rename them :)

GuillaumeGomez · 2025-02-10T19:49:17Z

src/librustdoc/html/format.rs

 }
+/// This macro is the same as [`std::writeln!`] for [`String`]s, but swallows the returned `Result`,
+/// since writing into a `String` can never fail.
+macro_rules! writeln_str {


Same here, make a function instead.

GuillaumeGomez · 2025-02-11T15:27:50Z

src/librustdoc/html/format.rs

-    pub(crate) fn len(&self) -> usize {
-        self.buffer.len()
-    }
+pub(crate) fn writeln_str(s: &mut String, f: fmt::Arguments<'_>) {


Just realized but if we always use format_args as argument, we can ismply put the \n directly into it so we don't need this function and not need to have s.push('\n') either.

Yep, that's true.
The one reason for having a separate writeln_str function is that it'll be easier to move to writeln! once we stop passing &mut Strings around. The codebase currently has instances of both writeln! and write!("...\n").
Since another call to .push('\n') might be bad for perf (it could cause reallocations), what we could do in the meantime is delete the writeln_str fn and use write_str with the (unstable) format_args_nl macro.
Or we could just add \ns to format strings :)
Whatever you prefer.

Let's use write_str with format_args_nl then.

GuillaumeGomez · 2025-02-11T15:51:24Z

Looks good to me, thanks! r=me once CI pass

yotamofek · 2025-02-11T15:53:44Z

Thanks! Rebased everything into one commit.

GuillaumeGomez · 2025-02-11T16:58:10Z

Awesome!

Let's go then. :)

@bors r+ rollup

bors · 2025-02-11T16:58:13Z

📌 Commit 39eb5e9 has been approved by GuillaumeGomez

It is now in the queue for this repository.

…-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

yotamofek · 2025-02-12T18:23:48Z

Had a merge conflict with #136829 , gonna need another approval, I think @GuillaumeGomez / @aDotInTheVoid (this PR is not listed as "approved" on the homu queue anymore)

GuillaumeGomez · 2025-02-12T18:33:54Z

@bors r+ rollup

bors · 2025-02-12T18:33:58Z

📌 Commit d99d8c2 has been approved by GuillaumeGomez

It is now in the queue for this repository.

…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#136758 (tests: `-Copt-level=3` instead of `-O` in assembly tests) - rust-lang#136761 (tests: `-Copt-level=3` instead of `-O` in codegen tests) - rust-lang#136784 (Nuke `Buffer` abstraction from `librustdoc`, take 2 💣) - rust-lang#136838 (Check whole `Unsize` predicate for escaping bound vars) - rust-lang#136848 (add docs and ut for bootstrap util cache) - rust-lang#136871 (dev-guide: Link to `t-lang` procedures for new features) - rust-lang#136890 (Change swap_nonoverlapping from lang to library UB) - rust-lang#136901 (compiler: give `ExternAbi` truly stable `Hash` and `Ord`) - rust-lang#136907 (compiler: Make middle errors `pub(crate)` and bury the dead code) - rust-lang#136916 (use cc archiver as default in `cc2ar`) r? `@ghost` `@rustbot` modify labels: rollup

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.

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

yotamofek mentioned this pull request Feb 9, 2025

Nuke Buffer abstraction from librustdoc 💣 #136748

Closed

yotamofek force-pushed the pr/rustdoc-remove-buffer-take2 branch from 0b77581 to cbfbc2f Compare February 9, 2025 18:52

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

GuillaumeGomez requested changes Feb 10, 2025

View reviewed changes

yotamofek force-pushed the pr/rustdoc-remove-buffer-take2 branch from cbfbc2f to 9cb57c6 Compare February 11, 2025 15:25

GuillaumeGomez reviewed Feb 11, 2025

View reviewed changes

GuillaumeGomez approved these changes Feb 11, 2025

View reviewed changes

yotamofek force-pushed the pr/rustdoc-remove-buffer-take2 branch from 8aab3d1 to 39eb5e9 Compare February 11, 2025 15:53

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

GuillaumeGomez mentioned this pull request Feb 12, 2025

Rollup of 8 pull requests #136917

Closed

Nuke Buffer abstraction from librustdoc 💣

d99d8c2

yotamofek force-pushed the pr/rustdoc-remove-buffer-take2 branch from 39eb5e9 to d99d8c2 Compare February 12, 2025 16:01

GuillaumeGomez mentioned this pull request Feb 12, 2025

Rollup of 10 pull requests #136943

Merged

bors merged commit 10b5596 into rust-lang:master Feb 12, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Feb 12, 2025

yotamofek deleted the pr/rustdoc-remove-buffer-take2 branch February 12, 2025 23:44

yotamofek mentioned this pull request Feb 22, 2025

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 #136784

Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 #136784

yotamofek commented Feb 9, 2025

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

GuillaumeGomez Feb 10, 2025

yotamofek Feb 10, 2025

GuillaumeGomez Feb 11, 2025

yotamofek Feb 11, 2025

GuillaumeGomez Feb 10, 2025

GuillaumeGomez Feb 11, 2025

yotamofek Feb 11, 2025

GuillaumeGomez Feb 11, 2025

yotamofek Feb 11, 2025

GuillaumeGomez commented Feb 11, 2025

yotamofek commented Feb 11, 2025

GuillaumeGomez commented Feb 11, 2025

bors commented Feb 11, 2025

yotamofek commented Feb 12, 2025

GuillaumeGomez commented Feb 12, 2025

bors commented Feb 12, 2025

Nuke Buffer abstraction from librustdoc, take 2 💣 #136784

Nuke Buffer abstraction from librustdoc, take 2 💣 #136784

Conversation

yotamofek commented Feb 9, 2025

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 and improvements - please read the text below

yotamofek commented Feb 10, 2025

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

GuillaumeGomez commented Feb 11, 2025

yotamofek commented Feb 11, 2025

GuillaumeGomez commented Feb 11, 2025

bors commented Feb 11, 2025

yotamofek commented Feb 12, 2025

GuillaumeGomez commented Feb 12, 2025

bors commented Feb 12, 2025

Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 #136784

Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 #136784