Get piece unchecked in write

[camsteffen]

We already use specialized zip, but it seems like we can do a little better by not checking pieces length at all.

Arguments constructors are now unsafe. So the format_args! expansion now includes an unsafe block.

Local Bench Diff

 name before ns/iter after ns/iter diff ns/iter diff % speedup fmt::write_str_macro1 22,967 19,718 -3,249 -14.15% x 1.16 fmt::write_str_macro2 35,527 32,654 -2,873 -8.09% x 1.09 fmt::write_str_macro_debug 571,953 575,973 4,020 0.70% x 0.99 fmt::write_str_ref 9,579 9,459 -120 -1.25% x 1.01 fmt::write_str_value 9,573 9,572 -1 -0.01% x 1.00 fmt::write_u128_max 176 173 -3 -1.70% x 1.02 fmt::write_u128_min 138 134 -4 -2.90% x 1.03 fmt::write_u64_max 139 136 -3 -2.16% x 1.02 fmt::write_u64_min 129 135 6 4.65% x 0.96 fmt::write_vec_macro1 24,401 22,273 -2,128 -8.72% x 1.10 fmt::write_vec_macro2 37,096 35,602 -1,494 -4.03% x 1.04 fmt::write_vec_macro_debug 588,291 589,575 1,284 0.22% x 1.00 fmt::write_vec_ref 9,568 9,732 164 1.71% x 0.98 fmt::write_vec_value 9,516 9,625 109 1.15% x 0.99 

[rust-highfive]

r? @sfackler

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

[joshtriplett]

This seems a little surprising. Iterators like args.pieces.iter() should not need to do bounds checks, in general; they know their own bounds.

[camsteffen]

This does not remove a bounds check within the loop. Only a length check of pieces when initializing ZipImpl.

[sfackler]

Arguments can be safely (if unstably) constructed, so this is unsound as-is: https://doc.rust-lang.org/src/core/fmt/mod.rs.html#317-332

[camsteffen]

I added assertions. I think that shouldn't be an issue, unless someone is using fmt_internals erroneously. Another local bench run was all the same or better.

[bors]

☔ The latest upstream changes (presumably #83580) made this pull request unmergeable. Please resolve the merge conflicts.

[camsteffen]

Now any unsafe block like format_args!("..", unsafe { .. }) is redundant which is causing test failures. To fix this, I think I would have to change format_args! to expand to, for example,

// format_args!("hello {}", world)matchmatch(&world,){(arg0,) => [::core::fmt::ArgumentV1::new(
arg0,::core::fmt::Display::fmt,)],}{ref args => unsafe{::core::fmt::Arguments::new_v1(&["hello "], args)},};

Thoughts?

[crlf0710]

@camsteffen Ping from triage, CI is still red here. Maybe you can use any change necessary to get CI green, so we can pass this on to the reviewer?

[JohnCSimon]

camsteffen requested a review from sfackler 16 days ago

@rustbot label: +S-waiting-on-review -S-waiting-on-author

[camsteffen]

Error seems unrelated and I couldn't reproduce locally. @bors retry

[bors]

⌛ Testing commit b1e07b8 with merge de42550...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing de42550 to master...

[rylev]

This caused a large regression in instruction counts when testing the compiler. This is a largely a change in std lib code, and we don't really have a good process for dealing with how std lib changes effect performance.

This seems to be primarily affecting debug and check builds, but there doesn't seem to be a query that is clearly to blame here. Given the motivation of this PR is primarily performance, I think it deserves a closer look.

As part of the performance triage process, I'm marking this as a performance regression. If you believe this performance regression to be justifiable or once you have an issue or PR that addresses this regression, please mark this PR with the label perf-regression-triaged.

@rustbot label +perf-regression

[camsteffen]

The regression seems to be caused by the change in the format_args! expansion which is now more complex in order to have an unsafe block that surrounds Arguments::new_v1 but does not surround the format_args! macro input. I reverted that particular change as an experiment in #88571 and that seemed to fix the perf regression. I don't know how to get a closer look at what is going on with style-servo-debug and would be interested in any pointers.

[Mark-Simulacrum]

Hm. So #88571 does look to fix the majority of the regression here, but I suppose we cannot land it as-is due to the unsafe nesting causing some problems with spurious lints.

I would lean towards reverting this PR; the small wins in runtime performance do not seem worth it given that fmt isn't optimized for performance anyway (indirect calls, can't get inlined, etc.). The compile time regressions are not insignificant.

[camsteffen]

My only reservation with reverting is that the internal fmt API was already unsafe before this PR added the unsafe modifiers.

[Mark-Simulacrum]

I.. sort of doubt that the get_unchecked there matters that much either, so I'd at least be fine with dropping it too (in favor of safety) as part of a pseudo-revert.

[Mark-Simulacrum]

Looks like the justification is noted in the commit (so nice!) -- d80f127. I think that makes sense, but we could likely get away with something "not wrong" as a result without invoking the full panic machinery. e.g., get().unwrap_or("") may work OK in some cases.

We could also move the unsafety as an assertion into a ZST that the caller must construct via an unsafe method, which would get compiled out but also allow us to put the unsafe block in a more local place, rather than wrapping the whole function call.

[camsteffen]

@Mark-Simulacrum I'm thinking the next step here for perf triage is to do a perf run reverting this and the follow-up PRs?

[Mark-Simulacrum]

I think #89139 definitely helped with some of the regressions caused by this PR. I think a trial revert would still be good, though, to estimate how much of a performance hit this is with the mitigations in place (like ZST-based unsafe).

[camsteffen]

The revert perf run looks good! #89521 (comment)

[Mark-Simulacrum]

Hm, the revert results feel a little unexpected. Unless there's been unrelated performance optimization that mitigates much of the regression seen in this PR, we would expect the revert to look equivalent, right? But instead we see barely any improvement.