Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few minor write_str optimizations #2697

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Feb 12, 2024

Apparently write! generates more code than write_str when used with a simple string, so optimizing it.

Apparently `write!` generates more code than `write_str` when used with a simple string, so optimizing it.
@oli-obk
Copy link
Member

oli-obk commented Feb 12, 2024

This sounds like something we could fix directly in libstd. Do you know of any attempts of doing that, or why it hasn't been done? We do similar trickery for format! after all

@nyurik
Copy link
Contributor Author

nyurik commented Feb 12, 2024

I tried it with https://rust.godbolt.org/z/Y8djWsq1P

This issue is also tracked in rust-lang/rust#99012 - but it appears to be highly complex and nowhere near being solved in a near future, thus warranting a workaround at least in the popular crates.

@nyurik nyurik changed the title A few minor write_str optimizations and inlining A few minor write_str optimizations Feb 12, 2024
@dtolnay
Copy link
Member

dtolnay commented Feb 13, 2024

(I have not read most of rust-lang/rust#99012.)

As I understand it, write!(f, "...") does not optimize in a straightforward way to f.write_str("...") because:

  • write! does not get to assume f implements core::fmt::Write, or std::io::Write. The only thing it gets to assume about f is that f.write_fmt(...) is callable and takes 1 argument of type core::fmt::Arguments. It would not be backward compatible for write!(f, "...") to expand to anything other than f.write_fmt(...).

  • We do not want any part of any write_fmt method from the standard library to be inlined. They are called so often that this is practically guaranteed to regress compile times.

I think there is a libstd fix and a compiler fix that are worth trying:

  1. Standard library: change core::fmt::Formatter::write_fmt (and maybe others) to the following, and see how bad the regression is.

    + #[inline]
      pub fn write_fmt(&mut self, f: fmt::Arguments) -> fmt::Result {
    +     if let ([s], []) = (f.pieces, f.args) {
    +         self.buf.write_str(s)
    +     } else {
              write(self.buf, f)
    +     }
      }
  2. Compiler voodoo. Make write! lower directly to a dedicated AST node, the way format_args! does since Move format_args!() into AST (and expand it during AST lowering) rust-lang/rust#106745. If the receiver ends up being core::fmt::Formatter (or one of a small hardcoded set of other types) then it turns into write_str, otherwise it needs to be handled in a way that is indistinguishable from f.write_fmt(...) — no idea how feasible this is.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks nyurik!

@dtolnay dtolnay merged commit 1d54973 into serde-rs:master Feb 13, 2024
15 checks passed
@nyurik nyurik deleted the format-str branch February 13, 2024 03:50
@nyurik
Copy link
Contributor Author

nyurik commented Feb 13, 2024

@dtolnay thx so much for the in-depth write-up! TIL. I can try the first approach (relatively simple). The second one clearly involves a lot more in-depth compiler knowledge... maybe someday

@nyurik
Copy link
Contributor Author

nyurik commented Feb 13, 2024

P.S. Turns out Arguments::as_str already does all the needed detections!

nyurik added a commit to nyurik/rust that referenced this pull request Feb 13, 2024
Per @dtolnay suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```
nyurik added a commit to nyurik/rust that referenced this pull request Feb 13, 2024
Per @dtolnay suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2024
perf: improve write_fmt to handle simple strings

Per `@dtolnay` suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
+ #[inline]
  pub fn write_fmt(&mut self, f: fmt::Arguments) -> fmt::Result {
+     if let Some(s) = f.as_str() {
+         self.buf.write_str(s)
+     } else {
          write(self.buf, f)
+     }
  }
```

Hopefully it will improve the simple case for the rust-lang#99012

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
@nyurik
Copy link
Contributor Author

nyurik commented Feb 13, 2024

I tried the above suggestion, and it seems to already produce a significantly smaller assembly as seen in rust-lang/rust#121001 (comment)

My only concern is that it will impact either the build time or the bin size in some other case... guess will have to wait for the results...

jonasbb added a commit to jonasbb/serde_with that referenced this pull request Feb 13, 2024
write! with a single string argument is not properly optimized and using
write_str generates better code:
serde-rs/serde#2697
rust-lang/rust#121001
jonasbb added a commit to jonasbb/serde_with that referenced this pull request Feb 13, 2024
write! with a single string argument is not properly optimized and using
write_str generates better code:
serde-rs/serde#2697
rust-lang/rust#121001
nyurik added a commit to nyurik/rust that referenced this pull request Feb 14, 2024
Per @dtolnay suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```
nyurik added a commit to nyurik/rust that referenced this pull request Feb 14, 2024
Per @dtolnay suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024
perf: improve write_fmt to handle simple strings

Per `@dtolnay` suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
+ #[inline]
  pub fn write_fmt(&mut self, f: fmt::Arguments) -> fmt::Result {
+     if let Some(s) = f.as_str() {
+         self.buf.write_str(s)
+     } else {
          write(self.buf, f)
+     }
  }
```

* Hopefully it will improve the simple case for the rust-lang#99012
* Another related (original?) issues rust-lang#10761
* Previous similar attempt to fix it by by `@Kobzol` rust-lang#100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
nyurik added a commit to nyurik/rust that referenced this pull request Feb 20, 2024
Per @dtolnay suggestion in serde-rs/serde#2697 (comment) - attempt to speed up performance in the cases of a simple string format without arguments:

```rust
write!(f, "text")  ->  f.write_str("text")
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
perf: improve write_fmt to handle simple strings

In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string.

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;
```

* Hopefully it will improve the simple case for the rust-lang#99012
* Another related (original?) issues rust-lang#10761
* Previous similar attempt to fix it by by `@Kobzol` rust-lang#100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
perf: improve write_fmt to handle simple strings

In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string.

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;
```

* Hopefully it will improve the simple case for the rust-lang#99012
* Another related (original?) issues rust-lang#10761
* Previous similar attempt to fix it by by `@Kobzol` rust-lang#100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
perf: improve write_fmt to handle simple strings

In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string.

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;
```

* Hopefully it will improve the simple case for the rust-lang#99012
* Another related (original?) issues rust-lang#10761
* Previous similar attempt to fix it by by `@Kobzol` rust-lang#100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 6, 2024
perf: improve write_fmt to handle simple strings

In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string.

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;
```

* Hopefully it will improve the simple case for the rust-lang/rust#99012
* Another related (original?) issues #10761
* Previous similar attempt to fix it by by `@Kobzol` #100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
perf: improve write_fmt to handle simple strings

In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string.

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;
```

* Hopefully it will improve the simple case for the rust-lang/rust#99012
* Another related (original?) issues rust-lang#10761
* Previous similar attempt to fix it by by `@Kobzol` #100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
perf: improve write_fmt to handle simple strings

In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string.

```rust
write!(f, "text")  ->  f.write_str("text")
```

```diff
 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;
```

* Hopefully it will improve the simple case for the rust-lang/rust#99012
* Another related (original?) issues rust-lang#10761
* Previous similar attempt to fix it by by `@Kobzol` #100700

CC: `@m-ou-se` as probably the biggest expert in everything `format!`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants