Specialize format! for simple "{}" case #76531

workingjubilee · 2020-09-09T17:55:38Z

If the first argument to format! is "{}" then the rest of the args must
be something that implements fmt::Display and thus ToString. We can thus
call ToString on it. In order to type-safely eat all those arguments,
a struct wrapper inside a closure prevents misuse.

This fixes #52804.

If the first argument to format! is "{}" then the rest of the args must be something that implements fmt::Display and thus ToString. We can thus call ToString on it. In order to type-safely eat all those arguments, a struct wrapper inside a closure prevents misuse.

rust-highfive · 2020-09-09T17:55:41Z

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

workingjubilee · 2020-09-09T18:20:17Z

Initial experiments with measureme's toolset suggest mixed results due to more effort in expanding this macro, but because the resulting code is often more familiar to Rust's existing patterns, even with this bespoke type guard, it can sometimes optimize better (or at least with less effort), but I don't have things set up to run any comprehensive/conclusive experiments yet.

notriddle · 2020-09-09T20:50:22Z

Why not put NeedsDisplay outside of the macro body, hide it with #[unstable] and getting at it with #[allow_internal_unstable]? This way, you don't have to expand a fresh trait on every invocation.

For example, the panic! macro does this:

rust/library/std/src/macros.rs

Line 10 in d92155b

#[allow_internal_unstable(libstd_sys_internals)]

rust/library/std/src/panicking.rs

Line 424 in d92155b

    
           #[unstable(feature = "libstd_sys_internals", reason = "used by the panic! macro", issue = "none")]

This uses #[unstable] and #[allow_internal_unstable] to reduce the need for expansion of the struct in every macro invocation.

workingjubilee · 2020-09-10T00:51:12Z

Ah, I am not as familiar with that sort of thing yet! It seems like a good idea.

Mark-Simulacrum

I am personally unsure that we want to do this. I think clippy already lints on format!("{}", foo) to suggest foo.to_string().

I would prefer that we at least move the fast closure to an associated function on NeedsDisplay, not a closure, to avoid generating more code than is necessary.

Mark-Simulacrum · 2020-09-11T17:32:18Z

library/alloc/src/macros.rs

+        let fast = |t: NeedsDisplay<_> | $crate::string::ToString::to_string(t.inner);
+        // This TokenTree must resolve to a Displayable value to be valid.
+        fast(NeedsDisplay { inner: &{$($arg)*} })
+    }};


We will want a codegen test I think for this, at least -- it looks like this is just not triggering to me, because I would expect NeedsDisplay to not resolve here. Generally we need the type to be public in order for it to be constructible outside the current crate.

codegen – tests that compile and then test the generated LLVM code to make sure that the optimizations we want are taking effect. See LLVM docs for how to write such tests.

Ah, one of these? Well, that just made this PR more Exciting.

Note that before working on that I would really fix this macro to make sure this arm is getting expanded, because right now that seems pretty unlikely to me. See also the comment below about needing :ident on the arm, most likely, which'll further simplify this branch.

scottmcm · 2020-09-12T01:31:20Z

library/alloc/src/macros.rs

 #[macro_export]
 #[stable(feature = "rust1", since = "1.0.0")]
 macro_rules! format {
+    // A faster path for simple format! calls.
+    ("{}", $($arg:tt,?)+) => {{


nit: does this need to deal with a full $($arg:tt,?)+? Maybe it could just take $e:expr ,? and let all the more complicated cases still be handled by the other arms of the macro.

(I don't think there's a need to optimize format!("{}", 1, 2, 3, 4, 5, 6).)

What's worse than that: even the expr version will not be correct.

Right now, format! parses named arguments, like a = 1. If that was parsed as an expression, then it would evaluate to (), and this would not pass the assertion.

let a = format!("{}", x = 1); assert_eq!(&a, "1");

I think it actually needs to be $arg:ident, and the optimization probably wouldn't be triggered very often as a result. That's annoying.

Oh wow, I didn't think about that kind of usage.
@notriddle Can you post or link me to some more... uh... weird examples like that? I mean, it might make me actually just close this PR but I'd like to have an opportunity to fool around with the alternate cases to see how I can handle it in the pattern matching and if it destroys the gains.

https://doc.rust-lang.org/std/fmt/index.html#usage

Right, I'm aware of the usual usages, but that doesn't include any examples that I can see which match the format!("{}", x = 1); case that I can see, I've never seen a named parameter then being passed to an "anonymous" formatting parameter.

but that doesn't include any examples that I can see which match the format!("{}", x = 1); case that I can see

That's because it's a bug and shouldn't be accepted: #45256

Dylan-DPC-zz · 2020-10-08T00:44:00Z

@workingjubilee any updates?

workingjubilee · 2020-10-21T20:31:56Z

I can't believe that worked.

crlf0710 · 2020-11-06T11:49:31Z

Triage: Seems ready for review.

Mark-Simulacrum · 2020-11-06T20:11:40Z

r? @m-ou-se who has some relevant recent context, I think

m-ou-se · 2020-11-06T21:22:40Z

You'll have to do the same { let res = ...; res } pattern that the macro is already expanding to. Without that, this change would break the following:

async fn f(_: String) {}

fn g() -> impl Send {
    async move {
        let x = 123;
        f(format!("{}", x)).await;
    }
}

the trait `Sync` is not implemented for `dyn std::fmt::Display`

future is not `Send` as this value is used across an await

         $crate::fmt::Display::to_string(&$arg);
                                         ----- has type `&dyn std::fmt::Display` which is not `Send`

See #64477 (comment).

You can make it work for all expressions instead of only identifiers by making a separate case to handle ident = expr:

macro_rules! format {
    ("{}", $name:ident = $arg:expr $(,)?) => {{
        let res = $crate::fmt::Display::to_string(&$arg);
        res
    }};
    ("{}", $arg:expr $(,)?) => {{
        let res = $crate::fmt::Display::to_string(&$arg);
        res
    }};
    ...
}

(I also added $(,)? here, to allow for a trailing comma.)

To make sure any future lint like suggested in #45256 (comment) would still warn about the first case, you could add a let _ = format_args!("{}", $name = $arg); there. (Although right now that has no effect. Alternatively, you could just add a note to #45256 to remind the future implementer of that lint to take a look at format!("{}", name = expr) too.)

m-ou-se · 2020-11-06T21:30:30Z

It'd be good to add some tests. Specifically to:

Check format!("{}", thing) still formats the thing correctly. (library/alloc/tests/fmt.rs)
Check the await/Send example mentioned above still works. (src/test/ui/async-await/issues/issue-64477-2.rs)
Check that format!("{}", thing) still does not compile if thing implements ToString but not Display.
Check with a codegen test that format!("{}", thing) does no longer call alloc::fmt::format or use any Display implementation like it did before this change.

m-ou-se · 2020-11-06T21:34:44Z

I don't think $crate::fmt::Display::to_string(&$arg); is calling the specialized ToString::to_string like you want.

Display::to_string seems to refer to the ToString::to_string implementation for dyn Display. I'm not completely sure, but I think format!("{}", String::new()) would now create a &dyn Display to that String and call the generic Display::to_string on that trait object, instead of calling the specialized <String as ToString>::to_string.

JohnCSimon · 2020-11-24T02:25:57Z

Ping from triage:
@workingjubilee - can you please address the comments from m-ou-se? Thanks

workingjubilee · 2020-12-02T08:37:01Z

Aye aye, capitán.

crlf0710 · 2020-12-18T11:21:53Z

@workingjubilee Ping from triage, any updates on this? If there's still questions welcome to talk about it over on official Zulip! Thanks.

Dylan-DPC-zz · 2021-01-12T20:43:10Z

@workingjubilee any updates?

workingjubilee · 2021-01-28T23:09:20Z

My sincere apologies, shortly after my last post the very computer I sent it from broke. I now have surplus compute again and I would like to try once more to take a look at this, though if another strike of unluck hits me I will try to reply in a more timely fashion and quit the field, at least on this PR, rather than leaving people waiting any further (and leaving myself open to more lightning bolts from Olympus).

crlf0710 · 2021-03-05T18:32:06Z

@workingjubilee Triage: what's the current status?

jyn514 · 2021-03-05T19:46:31Z

library/alloc/src/macros.rs

@@ -102,6 +102,10 @@ macro_rules! vec {
 #[macro_export]
 #[stable(feature = "rust1", since = "1.0.0")]
 macro_rules! format {
+    // A faster path for simple format! calls.
+    ("{}", $arg:ident) => {{


Any reason to make this so restrictive? I think $arg:expr $(,)? would catch a lot more and still allow calling to_string.

#76531 (comment)

What's worse than that: even the expr version will not be correct.

Right now, format! parses named arguments, like a = 1. If that was parsed as an expression, then it would evaluate to (), and this would not pass the assertion.

let a = format!("{}", x = 1); assert_eq!(&a, "1");

I think it actually needs to be $arg:ident, and the optimization probably wouldn't be triggered very often as a result. That's annoying.

Dylan-DPC-zz · 2021-03-11T02:12:44Z

Closing this due to inactivity.

rust-highfive assigned Mark-Simulacrum Sep 9, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2020

Minimize expansion on NeedsDisplay

4e45df7

This uses #[unstable] and #[allow_internal_unstable] to reduce the need for expansion of the struct in every macro invocation.

Mark-Simulacrum reviewed Sep 11, 2020

View reviewed changes

scottmcm reviewed Sep 12, 2020

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2020

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2020

Simplify to invoking Trait on ident

1ee0392

workingjubilee force-pushed the try-simple-format branch from ceee28d to 1ee0392 Compare October 21, 2020 19:45

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2020

rust-highfive assigned m-ou-se and unassigned Mark-Simulacrum Nov 6, 2020

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2020

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021

jyn514 reviewed Mar 5, 2021

View reviewed changes

Dylan-DPC-zz closed this Mar 11, 2021

Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2021

markus-bauer mentioned this pull request Feb 12, 2022

String formatting is slow. Commands like Print should write strings directly and not use write!. crossterm-rs/crossterm#628

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specialize format! for simple "{}" case #76531

Specialize format! for simple "{}" case #76531

workingjubilee commented Sep 9, 2020

rust-highfive commented Sep 9, 2020

workingjubilee commented Sep 9, 2020

notriddle commented Sep 9, 2020 •

edited

Loading

workingjubilee commented Sep 10, 2020

Mark-Simulacrum left a comment

Mark-Simulacrum Sep 11, 2020

workingjubilee Sep 15, 2020 •

edited

Loading

Mark-Simulacrum Sep 15, 2020

scottmcm Sep 12, 2020

notriddle Sep 12, 2020 •

edited

Loading

workingjubilee Sep 15, 2020

notriddle Sep 15, 2020

workingjubilee Sep 16, 2020

ollie27 Sep 16, 2020

Dylan-DPC-zz commented Oct 8, 2020

workingjubilee commented Oct 21, 2020

crlf0710 commented Nov 6, 2020

Mark-Simulacrum commented Nov 6, 2020

m-ou-se commented Nov 6, 2020

m-ou-se commented Nov 6, 2020 •

edited

Loading

m-ou-se commented Nov 6, 2020

JohnCSimon commented Nov 24, 2020

workingjubilee commented Dec 2, 2020

crlf0710 commented Dec 18, 2020

Dylan-DPC-zz commented Jan 12, 2021

workingjubilee commented Jan 28, 2021

crlf0710 commented Mar 5, 2021

jyn514 Mar 5, 2021

notriddle Mar 5, 2021 •

edited

Loading

Dylan-DPC-zz commented Mar 11, 2021

Specialize format! for simple "{}" case #76531

Specialize format! for simple "{}" case #76531

Conversation

workingjubilee commented Sep 9, 2020

rust-highfive commented Sep 9, 2020

workingjubilee commented Sep 9, 2020

notriddle commented Sep 9, 2020 • edited Loading

workingjubilee commented Sep 10, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle Sep 12, 2020 • edited Loading

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

Dylan-DPC-zz commented Oct 8, 2020

workingjubilee commented Oct 21, 2020

crlf0710 commented Nov 6, 2020

Mark-Simulacrum commented Nov 6, 2020

m-ou-se commented Nov 6, 2020

m-ou-se commented Nov 6, 2020 • edited Loading

m-ou-se commented Nov 6, 2020

JohnCSimon commented Nov 24, 2020

workingjubilee commented Dec 2, 2020

crlf0710 commented Dec 18, 2020

Dylan-DPC-zz commented Jan 12, 2021

workingjubilee commented Jan 28, 2021

crlf0710 commented Mar 5, 2021

Choose a reason for hiding this comment

notriddle Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Dylan-DPC-zz commented Mar 11, 2021

notriddle commented Sep 9, 2020 •

edited

Loading

workingjubilee Sep 15, 2020 •

edited

Loading

notriddle Sep 12, 2020 •

edited

Loading

m-ou-se commented Nov 6, 2020 •

edited

Loading

notriddle Mar 5, 2021 •

edited

Loading