librustdoc: impl core::fmt::Write for rustdoc::html::render::Buffer #92758

mfrw · 2022-01-11T07:41:27Z

Signed-off-by: Muhammad Falak R Wani [email protected]

Fixes: #92563

rust-highfive · 2022-01-11T07:41:31Z

r? @CraftSpider

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

mfrw · 2022-01-11T07:43:01Z

r? @jyn514

djc · 2022-01-11T08:33:01Z

src/librustdoc/html/format.rs

@@ -62,6 +62,12 @@ crate struct Buffer {
    buffer: String,
 }

+impl core::fmt::Write for Buffer {


I would probably just forward all three methods directly (I'm unsure about performance effects), since it's pretty trivial to do, and also annotate each of them with #[inline].

And we should probably remove the inherent write_str() and write_fmt() methods, replacing their usage with these?

I forwarded all the methods and then tried to use the trait methods but i could not make head or tail of the errors that came :(
I then tried to call the traits as <Self as Write>::write_fmt(....) which worked. Have not pushed those changes up though, as I am not sure if they are sane :(

camelid · 2022-01-11T20:12:37Z

r? @GuillaumeGomez

mfrw · 2022-01-12T03:53:10Z

I tried to use the newly implemented write_str & write_fmt but faced a lot of issues during compilation.
The thing that worked for me is using something like this in impl Buffer :

impl Buffer {
  ...
  crate fn write_str(&mut self, s: &str) {
     <Self as Write>::write_str(self, s).unwrap();
  }

  crate fn write_fmt(&mut self, v: fmt::Arguments<'_>) {
     <Self as Write>::write_fmt(self, v).unwrap();
  }
  ...
}

Although, I am not sure, if this is the right approach :(
I request for some guidance please.

djc · 2022-01-12T09:01:48Z

I meant removing the write_str() or write_fmt() methods from impl Buffer, and instead have callers rely on the methods implemented through std::fmt::Write. Have you considered doing that?

mfrw · 2022-01-12T11:28:52Z

I meant removing the write_str() or write_fmt() methods from impl Buffer, and instead have callers rely on the methods implemented through std::fmt::Write. Have you considered doing that?

Yes, I tried that first, I was unable to get that working. Let me give it one more try.
This is what i did, please correct me if something does not feel right.

Remove the impld 'write_fmt & write_str` methods.
Use the Write::write_fmt & Write::write_str and unwrap their result, as the methods that were impl'd did not return anything.

djc · 2022-01-12T11:57:40Z

That sounds like the right approach! How did it not work? Did you get a compiler error? If so, what?

mfrw · 2022-01-17T05:01:38Z

That sounds like the right approach! How did it not work? Did you get a compiler error? If so, what?

I apologize, I was not taking into account that the signatures have changed & I might have to add a bunch of unwraps.
Now that I did the change, I am seeing a test-break, which I have no clue how to fix or investigate.
So I will spend some more time on learning :)
I am able to pass tests locally :)

mfrw · 2022-01-17T06:39:56Z

@bors try

GuillaumeGomez · 2022-01-17T12:59:14Z

Please remove your merge commits and instead use rebase. Once done, I'll start a perf check because I'm curious to see if there is any impact. Please ping once you have removed the merge commit.

djc · 2022-01-17T13:11:33Z

src/librustdoc/html/render/print_item.rs

@@ -140,7 +140,7 @@ pub(super) fn print_item(cx: &Context<'_>, item: &clean::Item, buf: &mut Buffer,
    };

    let heading = item_vars.render().unwrap();
-    buf.write_str(&heading);
+    buf.write_str(&heading).unwrap();


I think the original point of my issue was to change this to do buf.write_fmt(format!("{}", heading)).

Sure, let me do that activity after I resolve the git issues :)
[btw .. this process has been immense learning & apologies for me being a little slow to understand the bigger picture]

What about this @mfrw?

It's fine don't worry. If you have questions don't hesitate to ask. Thank you for working on this! :)

djc · 2022-01-17T13:23:49Z

I regret suggesting we should change all the instances of write_str() and write_fmt() to rely on the std::fmt::Write implementation, I now see that those methods were explicitly documented as avoiding unwrapping. @mfrw if you're up for it, maybe those should be reverted? @GuillaumeGomez what do you think?

mfrw · 2022-01-17T13:23:54Z

Please remove your merge commits and instead use rebase. Once done, I'll start a perf check because I'm curious to see if there is any impact. Please ping once you have removed the merge commit.

Take II with rebase instead of a merge.
Please take a look whenever you get a chance @GuillaumeGomez :)

mfrw · 2022-01-17T13:25:25Z

I regret suggesting we should change all the instances of write_str() and write_fmt() to rely on the std::fmt::Write implementation, I now see that those methods were explicitly documented as avoiding unwrapping. @mfrw if you're up for it, maybe those should be reverted? @GuillaumeGomez what do you think?

Sure, this was a huge learning :)
Although, I would love to see the perf results which @GuillaumeGomez mentioned.

GuillaumeGomez · 2022-01-17T13:26:39Z

I regret suggesting we should change all the instances of write_str() and write_fmt() to rely on the std::fmt::Write implementation, I now see that those methods were explicitly documented as avoiding unwrapping. @mfrw if you're up for it, maybe those should be reverted? @GuillaumeGomez what do you think?

If there is no perf improvement from it, I think that's what we will need to do because the increase of code for no gain isn't really worth it.

So for the perf now:

@bors try @rust-timer queue

rust-timer · 2022-01-17T13:26:40Z

Awaiting bors try build completion.

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

GuillaumeGomez · 2022-01-17T13:30:00Z

Please fix the error.

@bors: try-

EDIT: And by the time I wrote this message, you did. 😆

GuillaumeGomez · 2022-01-17T14:15:50Z

Ok, let's try again!

@bors try @rust-timer queue

mfrw · 2022-01-27T11:38:48Z

There's no real urgency, was just wondering! I'll give you some more time, let's see how it goes next week.

Thank you Very much :)

Signed-off-by: Muhammad Falak R Wani <[email protected]>

mfrw · 2022-02-01T13:17:21Z

@djc & @GuillaumeGomez apologies for the late turn around :(

djc

LGTM! Would be nice to get a perf run for this, I suppose?

GuillaumeGomez · 2022-02-01T15:45:20Z

Absolutely!

@bors try @rust-timer queue

rust-timer · 2022-02-01T15:45:22Z

Awaiting bors try build completion.

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

bors · 2022-02-01T15:45:28Z

⌛ Trying commit 62bea63 with merge 213368f0aa11b580455fc90c29d5e4b4700f73d1...

bors · 2022-02-01T18:10:06Z

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

rust-timer · 2022-02-01T18:10:08Z

Queued 213368f0aa11b580455fc90c29d5e4b4700f73d1 with parent 547f2ba, future comparison URL.

rust-timer · 2022-02-01T20:27:29Z

Finished benchmarking commit (213368f0aa11b580455fc90c29d5e4b4700f73d1): comparison url.

Summary: This benchmark run did not return any relevant results.

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

GuillaumeGomez · 2022-02-01T20:39:25Z

Thanks for all your work on this @mfrw!

@bors: r+ rollup

bors · 2022-02-01T20:39:27Z

📌 Commit 62bea63 has been approved by GuillaumeGomez

djc · 2022-02-01T20:42:21Z

I don't see any doc things in the rustc-perf listing, what am I missing?

GuillaumeGomez · 2022-02-01T20:49:44Z

You need to uncheck "Show only significant changes" and "Filter out very small changes". To have only relevant items, you can filter by using " doc".

…llaumeGomez librustdoc: impl core::fmt::Write for rustdoc::html::render::Buffer Signed-off-by: Muhammad Falak R Wani <[email protected]> Fixes: rust-lang#92563

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#92758 (librustdoc: impl core::fmt::Write for rustdoc::html::render::Buffer) - rust-lang#92788 (Detect `::` -> `:` typo in type argument) - rust-lang#93420 (Improve wrapping on settings page) - rust-lang#93493 (Document valid values of the char type) - rust-lang#93531 (Fix incorrect panic message in example) - rust-lang#93559 (Add missing | between print options) - rust-lang#93560 (Fix two incorrect "it's" (typos in comments)) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned CraftSpider Jan 11, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2022

djc reviewed Jan 11, 2022

View reviewed changes

rust-highfive assigned GuillaumeGomez and unassigned CraftSpider Jan 11, 2022

mfrw force-pushed the mfrw/rustdoc-impl-write branch 3 times, most recently from b284d10 to 31ea629 Compare January 17, 2022 06:28

djc reviewed Jan 17, 2022

View reviewed changes

mfrw force-pushed the mfrw/rustdoc-impl-write branch from eba5046 to ba4921d Compare January 17, 2022 13:21

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 17, 2022

This comment has been minimized.

Sign in to view

mfrw added 3 commits February 1, 2022 18:05

librustdoc: impl core::fmt::Write for rustdoc::html::render::Buffer

95bd87f

Signed-off-by: Muhammad Falak R Wani <[email protected]>

librustdoc: inline and forward all methods for impl Write for Buffer

66d7e50

Signed-off-by: Muhammad Falak R Wani <[email protected]>

librustdoc: render: use render_into instead of creating a temp string

62bea63

Signed-off-by: Muhammad Falak R Wani <[email protected]>

mfrw force-pushed the mfrw/rustdoc-impl-write branch from 0359e0b to 62bea63 Compare February 1, 2022 12:37

djc approved these changes Feb 1, 2022

View reviewed changes

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

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

GuillaumeGomez approved these changes Feb 1, 2022

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 1, 2022

matthiaskrgr mentioned this pull request Feb 2, 2022

Rollup of 8 pull requests #93570

Closed

matthiaskrgr mentioned this pull request Feb 2, 2022

Rollup of 7 pull requests #93573

Merged

bors merged commit 7117b45 into rust-lang:master Feb 2, 2022

rustbot added this to the 1.60.0 milestone Feb 2, 2022

librustdoc: impl core::fmt::Write for rustdoc::html::render::Buffer #92758

librustdoc: impl core::fmt::Write for rustdoc::html::render::Buffer #92758

Conversation

mfrw commented Jan 11, 2022

rust-highfive commented Jan 11, 2022

mfrw commented Jan 11, 2022 • edited Loading

djc Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

mfrw Jan 12, 2022

Choose a reason for hiding this comment

camelid commented Jan 11, 2022

mfrw commented Jan 12, 2022

djc commented Jan 12, 2022

mfrw commented Jan 12, 2022

djc commented Jan 12, 2022

mfrw commented Jan 17, 2022 • edited Loading

mfrw commented Jan 17, 2022

GuillaumeGomez commented Jan 17, 2022

djc Jan 17, 2022

Choose a reason for hiding this comment

mfrw Jan 17, 2022

Choose a reason for hiding this comment

GuillaumeGomez Jan 17, 2022

Choose a reason for hiding this comment

GuillaumeGomez Jan 17, 2022

Choose a reason for hiding this comment

djc commented Jan 17, 2022

mfrw commented Jan 17, 2022

mfrw commented Jan 17, 2022

GuillaumeGomez commented Jan 17, 2022

rust-timer commented Jan 17, 2022

This comment has been minimized.

GuillaumeGomez commented Jan 17, 2022 • edited Loading

GuillaumeGomez commented Jan 17, 2022

mfrw commented Jan 27, 2022

mfrw commented Feb 1, 2022

djc left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 1, 2022

rust-timer commented Feb 1, 2022

bors commented Feb 1, 2022

bors commented Feb 1, 2022

rust-timer commented Feb 1, 2022

rust-timer commented Feb 1, 2022

GuillaumeGomez commented Feb 1, 2022

bors commented Feb 1, 2022

djc commented Feb 1, 2022

GuillaumeGomez commented Feb 1, 2022

mfrw commented Jan 11, 2022 •

edited

Loading

djc Jan 11, 2022 •

edited

Loading

mfrw commented Jan 17, 2022 •

edited

Loading

GuillaumeGomez commented Jan 17, 2022 •

edited

Loading