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

Small perf improvement for fmt #57537

Merged
merged 3 commits into from Jan 22, 2019

Conversation

Projects
None yet
5 participants
@sinkuu
Copy link
Contributor

sinkuu commented Jan 12, 2019

Added benchmark is based on #10761

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 12, 2019

r? @alexcrichton

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

@sinkuu sinkuu force-pushed the sinkuu:fmt_perf branch from 77b3013 to 98a9c76 Jan 12, 2019

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jan 12, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0e5d1dd1:start=1547272729970290050,finish=1547272803403466501,duration=73433176451
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

sinkuu added some commits Jan 11, 2019

Utilize specialized zip iterator impl
 name                        old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 fmt::write_str_macro1       13,927       12,489             -1,438  -10.33%   x 1.12
 fmt::write_str_macro2       24,633       23,418             -1,215   -4.93%   x 1.05
 fmt::write_str_macro_debug  234,633      233,092            -1,541   -0.66%   x 1.01
 fmt::write_str_ref          5,819        5,823                   4    0.07%   x 1.00
 fmt::write_str_value        6,012        5,828                -184   -3.06%   x 1.03
 fmt::write_vec_macro1       18,550       17,143             -1,407   -7.58%   x 1.08
 fmt::write_vec_macro2       30,369       28,920             -1,449   -4.77%   x 1.05
 fmt::write_vec_macro_debug  244,338      244,901               563    0.23%   x 1.00
 fmt::write_vec_ref          5,952        5,885                 -67   -1.13%   x 1.01
 fmt::write_vec_value        5,944        5,894                 -50   -0.84%   x 1.01
Fix simple formatting optimization
name                        old2 ns/iter  new2 ns/iter  diff ns/iter   diff %  speedup
fmt::write_str_macro1       12,295        12,308                  13    0.11%   x 1.00
fmt::write_str_macro2       24,079        21,451              -2,628  -10.91%   x 1.12
fmt::write_str_macro_debug  238,363       230,807             -7,556   -3.17%   x 1.03
fmt::write_str_ref          6,203         6,064                 -139   -2.24%   x 1.02
fmt::write_str_value        6,225         6,075                 -150   -2.41%   x 1.02
fmt::write_vec_macro1       17,144        17,121                 -23   -0.13%   x 1.00
fmt::write_vec_macro2       29,845        26,703              -3,142  -10.53%   x 1.12
fmt::write_vec_macro_debug  248,840       242,117             -6,723   -2.70%   x 1.03
fmt::write_vec_ref          5,954         6,438                  484    8.13%   x 0.92
fmt::write_vec_value        5,959         6,439                  480    8.06%   x 0.93

@sinkuu sinkuu force-pushed the sinkuu:fmt_perf branch from 98a9c76 to 038d837 Jan 12, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 14, 2019

Thanks for this! The first commit looks good to me (although somewhat requiring wizardry to fully understand, specializing iterators does make for varying performance...)

For the second commit though the code here is way out of cache for me (that optimization was added in 2014). Can you explain a bit what's going on there to help understand it?

@sinkuu

This comment has been minimized.

Copy link
Contributor Author

sinkuu commented Jan 14, 2019

For formats without non-default formatting params (like positions not in order ("{2} {0} {1}") or specs ("{:02X}")), Arguments::new_v1 constructor can be used instead of Arguments::new_v1_formatted. new_v1 constructs Arguments with fmt field being None, which activates the fast path in fmt::write.

This optimization should apply to "{}" (which equals to "{0}"), but there seems to be a overlook in #45807 (added ArgumentImplicitlyIs variant without updating the optimization logic).

Example

fn foo(s: &mut String) {
    // parsed as `ArgumentIs(0)`
    write!(s, "{0}", "foo");
    // parsed as `ArgumentImplicitlyIs(0)`
    write!(s, "{}", "foo");
}

Expansion before this PR:

fn foo(s: &mut String) {
    // parsed as `ArgumentIs(0)`
    s.write_fmt($crate::fmt::Arguments::new_v1(
        &[""],
        &match (&"foo",) {
            (arg0,) => [$crate::fmt::ArgumentV1::new(arg0, $crate::fmt::Display::fmt)],
        },
    ));
    // parsed as `ArgumentImplicitlyIs(0)`
    s.write_fmt($crate::fmt::Arguments::new_v1_formatted(
        &[""],
        &match (&"foo",) {
            (arg0,) => [$crate::fmt::ArgumentV1::new(arg0, $crate::fmt::Display::fmt)],
        },
        &[$crate::fmt::rt::v1::Argument {
            position: $crate::fmt::rt::v1::Position::At(0usize),
            format: $crate::fmt::rt::v1::FormatSpec {
                fill: ' ',
                align: $crate::fmt::rt::v1::Alignment::Unknown,
                flags: 0u32,
                precision: $crate::fmt::rt::v1::Count::Implied,
                width: $crate::fmt::rt::v1::Count::Implied,
            },
        }],
    ));
}

Expansion after this PR:

fn foo(s: &mut String) {
    // parsed as `ArgumentIs(0)`
    s.write_fmt($crate::fmt::Arguments::new_v1(
        &[""],
        &match (&"foo",) {
            (arg0,) => [$crate::fmt::ArgumentV1::new(arg0, $crate::fmt::Display::fmt)],
        },
    ));
    // parsed as `ArgumentImplicitlyIs(0)`
    s.write_fmt($crate::fmt::Arguments::new_v1(
        &[""],
        &match (&"foo",) {
            (arg0,) => [$crate::fmt::ArgumentV1::new(arg0, $crate::fmt::Display::fmt)],
        },
    ));
}
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 15, 2019

@bors: r+

Sounds good to me, thanks for explaining!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 15, 2019

📌 Commit 038d837 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019

Rollup merge of rust-lang#57537 - sinkuu:fmt_perf, r=alexcrichton
Small perf improvement for fmt

Added benchmark is based on rust-lang#10761

bors added a commit that referenced this pull request Jan 15, 2019

Auto merge of #57624 - Centril:rollup, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57352 (forbid manually impl'ing one of an object type's marker traits)
 - #57486 (Simplify `TokenStream` some more)
 - #57537 (Small perf improvement for fmt)
 - #57579 (Add core::iter::once_with())
 - #57587 (Add 'rustc-env:RUST_BACKTRACE=0' to const-pat-ice test)
 - #57608 (Simplify 'product' factorial example)
 - #57614 ([rustdoc] Fix crates filtering box not being filled)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 15, 2019

Rollup merge of rust-lang#57537 - sinkuu:fmt_perf, r=alexcrichton
Small perf improvement for fmt

Added benchmark is based on rust-lang#10761

bors added a commit that referenced this pull request Jan 15, 2019

Auto merge of #57627 - Centril:rollup, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57352 (forbid manually impl'ing one of an object type's marker traits)
 - #57537 (Small perf improvement for fmt)
 - #57579 (Add core::iter::once_with())
 - #57587 (Add 'rustc-env:RUST_BACKTRACE=0' to const-pat-ice test)
 - #57608 (Simplify 'product' factorial example)
 - #57614 ([rustdoc] Fix crates filtering box not being filled)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jan 15, 2019

Auto merge of #57627 - Centril:rollup, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #57253 (Make privacy checking, intrinsic checking and liveness checking incremental)
 - #57352 (forbid manually impl'ing one of an object type's marker traits)
 - #57537 (Small perf improvement for fmt)
 - #57579 (Add core::iter::once_with())
 - #57587 (Add 'rustc-env:RUST_BACKTRACE=0' to const-pat-ice test)
 - #57608 (Simplify 'product' factorial example)
 - #57614 ([rustdoc] Fix crates filtering box not being filled)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 15, 2019

@bors r-

Failed in #57627 due to toolstate change (clippy).
Let's revisit this when we can make toolstate breakages again.

sinkuu added a commit to sinkuu/rust-clippy that referenced this pull request Jan 19, 2019

Catch up with `format_args` change
Catches up with a change in rust-lang/rust#57537

Happened to fix a bug in `expect_fun_call`, that is the lint ignores more than
one arguments to `format`.

bors added a commit to rust-lang/rust-clippy that referenced this pull request Jan 19, 2019

Auto merge of #3674 - sinkuu:fmt_rustup, r=oli-obk
Catch up with `format_args` change

Catches up with a change in rust-lang/rust#57537. (Since the optimization is optional, this clippy PR can be merged before the rustc PR.)

Happened to fix a bug in `expect_fun_call`, that is the lint ignores more than
one arguments to `format`.

```
warning: use of `expect` followed by a function call
 --> src/main.rs:2:17
  |
2 |     Some("foo").expect(format!("{} {}", 1, 2).as_ref());
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{} {}", 1))`
  |
```
@sinkuu

This comment has been minimized.

Copy link
Contributor Author

sinkuu commented Jan 22, 2019

This is ready to go. Can you re-"r+"?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 22, 2019

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 22, 2019

📌 Commit 038d837 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Jan 22, 2019

Rollup merge of rust-lang#57537 - sinkuu:fmt_perf, r=alexcrichton
Small perf improvement for fmt

Added benchmark is based on rust-lang#10761

bors added a commit that referenced this pull request Jan 22, 2019

Auto merge of #57830 - Centril:rollup, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #57537 (Small perf improvement for fmt)
 - #57552 (Default images)
 - #57604 (Make `str` indexing generic on `SliceIndex`.)
 - #57667 (Fix memory leak in P::filter_map)
 - #57677 (const_eval: Predetermine the layout of all locals when pushing a stack frame)
 - #57791 (Add regression test for #54582)
 - #57798 (Corrected spelling inconsistency)
 - #57809 (Add powerpc64-unknown-freebsd)
 - #57813 (fix validation range printing when encountering undef)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 22, 2019

⌛️ Testing commit 038d837 with merge ad30e9a...

@bors bors merged commit 038d837 into rust-lang:master Jan 22, 2019

1 check was pending

homu Testing commit 038d8372244ab088ea186e10704e2bfc4e83f477 with merge ad30e9a6814b5d29dfbdcd85ecde48afcc94389b...
Details

@sinkuu sinkuu deleted the sinkuu:fmt_perf branch Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment