Use `str::to_owned` when `format!` has no formatting arguments #75894

tesuji · 2020-08-25T02:25:35Z

This fixes partially #75742 .
I would like to add must_use to fmt::format, String::from and str::into<String>
just like existing must_use on str::to_owned.

A disadvantage of this PR is that it generates more code for debug builds. In release builds, the branch as_str is optimized out.

library/alloc/src/fmt.rs

jyn514 · 2020-08-25T02:33:03Z

@bors try @rust-timer queue

rust-timer · 2020-08-25T02:33:05Z

Awaiting bors try build completion

bors · 2020-08-25T02:33:14Z

⌛ Trying commit ad93d9ce8b08e2521c90a634967f7bb7c4726c76 with merge dd456fc07604958cdc0baa3a831659ecc61b2610...

bors · 2020-08-25T03:16:55Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: dd456fc07604958cdc0baa3a831659ecc61b2610 (dd456fc07604958cdc0baa3a831659ecc61b2610)

rust-timer · 2020-08-25T03:16:57Z

Queued dd456fc07604958cdc0baa3a831659ecc61b2610 with parent 3cf8f69, future comparison URL.

rust-timer · 2020-08-25T05:38:35Z

Finished benchmarking try commit (dd456fc07604958cdc0baa3a831659ecc61b2610): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

library/alloc/src/macros.rs

tesuji · 2020-08-25T15:37:41Z

Codgen tests should be passed now.
I'm not sure we need another perf run that performed in #75894 (comment). Thank Joshua for starting the rust-timer bot for me.

r? @nagisa for review or reassignment.
@rustbot modify labels: +S-waiting-on-review

library/core/src/fmt/mod.rs

src/test/codegen/issue-75742-format_without_fmt_args.rs

library/core/src/fmt/mod.rs

library/alloc/src/macros.rs

tesuji · 2020-08-26T14:14:07Z

Thanks Ralf for pointing out my misconception about const fn.
I updated the comments in code to reflect that it's const prop will do the magic.

nagisa

The codegen tests seem sketchy to me for reasons outlined in the inline comments. It seems likely to be a kind of test that breaks on various medium-sized changes all the time. If the goal is to test that a function with just format!() gets optimised (as a regression test for the referenced test) to an empty function, test exactly for that with

CHECK-LABEL: foo
CHECK-NEXT: ret

The "pretty"-print test seems to be sufficient if you're looking to ensure that things expand to what you expect, no need to bring codegen into an equation.

I don’t have strong opinions either way on the main change in this PR though. It mildly seems to me like it is optimizing a wrong thing in a wrong place, but I haven’t thought about it too much.

src/test/codegen/issue-75742-format_without_fmt_args.rs

nagisa · 2020-08-30T23:35:42Z

library/alloc/src/macros.rs

+        // inside `format_args!` itself.
+        let r = match $crate::__export::format_args!($($arg)*) {
+            // FIXME: We hope that constant propagation will make LLVM optimize out
+            // this match.


Is this FIXME actionable at all? Is this really a FIXME?

Yeah, this is more like a HACK.

tesuji · 2020-08-31T06:14:06Z

gets optimised (as a regression test for the referenced test) to an empty function, test exactly for that with

Thank you. I did it now. between the LABEL and the "ret void" there is a label. I have to account for it.

The "pretty"-print test seems to be sufficient if you're looking to ensure that things expand to what you expect, no need to bring codegen into an equation.

I want the codegen test to ensure the match is optimized out.

It mildly seems to me like it is optimizing a wrong thing in a wrong place

Yeah, it is a hack to me too.

bors · 2020-09-04T01:52:53Z

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

nagisa · 2020-09-08T23:57:01Z

LGTM, r=me once rebased.

tesuji · 2020-09-27T13:46:09Z

Think about it more: it would be possible to write format! as a procedure macro
to generate more efficient code instead of using this hack.
But that's not normal work.

Anyway, we could open an issue to consider that possibility.

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

nagisa · 2020-09-27T14:27:17Z

@bors r+

bors · 2020-09-27T14:27:19Z

📌 Commit be1ea4b has been approved by nagisa

bors · 2020-09-27T14:46:55Z

⌛ Testing commit be1ea4b with merge a1661d41bedb9d8c1b2b8a184440119170fd2a15...

rust-log-analyzer · 2020-09-27T14:53:02Z

Your PR failed (pretty log, 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.

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 @rust-lang/infra. (Feature Requests)

bors · 2020-09-27T15:14:49Z

💔 Test failed - checks-actions

bjorn3 · 2020-09-27T15:18:00Z

More helpful log than rust-log-analyzer:

---- [codegen] codegen/issue-75742-format_without_fmt_args.rs stdout ----

error: verification with 'FileCheck' failed
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-75742-format_without_fmt_args/issue-75742-format_without_fmt_args.ll" "/checkout/src/test/codegen/issue-75742-format_without_fmt_args.rs"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
/checkout/src/test/codegen/issue-75742-format_without_fmt_args.rs:10:16: error: CHECK-NEXT: is not on the line after the previous match
// CHECK-NEXT: {{"_ZN[^:]+"}}:
               ^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-75742-format_without_fmt_args/issue-75742-format_without_fmt_args.ll:195:1: note: 'next' match was here
"_ZN62_$LT$alloc..alloc..Global$u20$as$u20$core..alloc..AllocRef$GT$7dealloc17hcbba748b2e896a43E.exit.i.i.i.i25": ; preds = %bb8
^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-75742-format_without_fmt_args/issue-75742-format_without_fmt_args.ll:176:32: note: previous match ended here
define void @format_wo_fmt_args() unnamed_addr #2 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
                               ^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-75742-format_without_fmt_args/issue-75742-format_without_fmt_args.ll:177:1: note: non-matching line after previous match is here
bb8:
^

Input file: /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-75742-format_without_fmt_args/issue-75742-format_without_fmt_args.ll
Check file: /checkout/src/test/codegen/issue-75742-format_without_fmt_args.rs

-dump-input=help explains the following input dump.

Input was:
<<<<<<
         .
         .
         .
       190:  %_4.i.i.i.i.i22 = icmp eq i64 %_1.sroa.4.0.copyload, 0
       191:  %.not.i.i.i.i23 = icmp eq i8* %_1.sroa.0.0.copyload, null
       192:  %or.cond.i.i.i.i24 = or i1 %.not.i.i.i.i23, %_4.i.i.i.i.i22
       193:  br i1 %or.cond.i.i.i.i24, label %bb17, label %"_ZN62_$LT$alloc..alloc..Global$u20$as$u20$core..alloc..AllocRef$GT$7dealloc17hcbba748b2e896a43E.exit.i.i.i.i25"
       194: 
       195: "_ZN62_$LT$alloc..alloc..Global$u20$as$u20$core..alloc..AllocRef$GT$7dealloc17hcbba748b2e896a43E.exit.i.i.i.i25": ; preds = %bb8
next:10     !~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                error: match on wrong line
       196:  tail call void @__rust_dealloc(i8* nonnull %_1.sroa.0.0.copyload, i64 %_1.sroa.4.0.copyload, i64 1) #7
       197:  br label %bb17
       198: 
       199: bb17: ; preds = %"_ZN62_$LT$alloc..alloc..Global$u20$as$u20$core..alloc..AllocRef$GT$7dealloc17hcbba748b2e896a43E.exit.i.i.i.i25", %bb8
       200:  %1 = bitcast %"std::string::String"* %r1 to i8*
         .
         .
         .
>>>>>>

------------------------------------------



failures:
    [codegen] codegen/issue-75742-format_without_fmt_args.rs

rust-log-analyzer · 2020-09-28T04:20:40Z

The job x86_64-gnu of your PR failed (pretty log, 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.

Initialized empty Git repository in /home/runner/work/rust/rust/.git/
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/75894/merge:refs/remotes/pull/75894/merge
---
Checking which error codes lack tests...
Found 430 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/src/ci/scripts/run-build-from-ci.sh:19: line longer than 100 chars
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo"
expected success, got: exit code: 1



failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test
Build completed unsuccessfully in 0:00:27
== clock drift check ==
  local time: Mon Sep 28 04:20:30 UTC 2020
  network time: Mon, 28 Sep 2020 04:20:30 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (8131) (python)

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 @rust-lang/infra. (Feature Requests)

tesuji · 2020-09-28T09:46:11Z

I got the LLVM-IR in #77285. Looks like the test is not reliable.
It passes some builders and falls in others.
Also passes locally for me.

Anyway, I don't think it is a right approach anymore.
Thanks everyone involving in this PR.

I'm going to close this PR if you all don't have objections.

jyn514 reviewed Aug 25, 2020

View reviewed changes

library/alloc/src/fmt.rs Outdated Show resolved Hide resolved

tesuji force-pushed the const-format branch 2 times, most recently from 97f3e13 to 7dfbda9 Compare August 25, 2020 13:29

jyn514 reviewed Aug 25, 2020

View reviewed changes

library/alloc/src/macros.rs Outdated Show resolved Hide resolved

tesuji changed the title ~~[PERF only] Using str::to_owned when format! has no formating arguments~~ [PERF only] Using str::to_owned when format! has no formatting arguments Aug 25, 2020

tesuji force-pushed the const-format branch from 7dfbda9 to 9313360 Compare August 25, 2020 14:23

tesuji changed the title ~~[PERF only] Using str::to_owned when format! has no formatting arguments~~ Use str::to_owned when format! has no formatting arguments Aug 25, 2020

tesuji marked this pull request as ready for review August 25, 2020 15:35

rust-highfive assigned nagisa Aug 25, 2020

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2020

tesuji force-pushed the const-format branch 3 times, most recently from 8510a43 to 2c01a4e Compare August 25, 2020 15:58

tesuji commented Aug 25, 2020

View reviewed changes

library/core/src/fmt/mod.rs Outdated Show resolved Hide resolved

pickfire reviewed Aug 26, 2020

View reviewed changes

src/test/codegen/issue-75742-format_without_fmt_args.rs Show resolved Hide resolved

tesuji force-pushed the const-format branch 2 times, most recently from 230fcd6 to 00eefac Compare August 26, 2020 03:44

RalfJung reviewed Aug 26, 2020

View reviewed changes

library/core/src/fmt/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 26, 2020

View reviewed changes

library/alloc/src/macros.rs Outdated Show resolved Hide resolved

tesuji force-pushed the const-format branch from 00eefac to 4611b52 Compare August 26, 2020 14:09

tesuji force-pushed the const-format branch from 4611b52 to a5c3190 Compare August 26, 2020 14:19

nagisa reviewed Aug 30, 2020

View reviewed changes

tesuji force-pushed the const-format branch from 51e6ac9 to 53a18b7 Compare August 31, 2020 06:08

workingjubilee mentioned this pull request Sep 8, 2020

format_args! is slow #76490

Open

crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 25, 2020

Use str::to_owned for format! without formatting arguments

be1ea4b

tesuji force-pushed the const-format branch from 53a18b7 to be1ea4b Compare September 27, 2020 13:42

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 27, 2020

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 Sep 27, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 27, 2020

tesuji added 2 commits September 28, 2020 04:07

[dnm] enable fail tests

eb0ca61

[dnm] enable fail tests

4c9b5a6

tesuji marked this pull request as draft September 28, 2020 04:16

tesuji mentioned this pull request Sep 28, 2020

[DO NOT MERGE] [ignore this] playing with CI #77285

Closed

tesuji closed this Sep 29, 2020

tesuji deleted the const-format branch September 29, 2020 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `str::to_owned` when `format!` has no formatting arguments #75894

Use `str::to_owned` when `format!` has no formatting arguments #75894

tesuji commented Aug 25, 2020 •

edited

Loading

jyn514 commented Aug 25, 2020

rust-timer commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 25, 2020

rust-timer commented Aug 25, 2020

rust-timer commented Aug 25, 2020

tesuji commented Aug 25, 2020

tesuji commented Aug 26, 2020

nagisa left a comment

nagisa Aug 30, 2020

tesuji Aug 31, 2020

tesuji commented Aug 31, 2020

bors commented Sep 4, 2020

nagisa commented Sep 8, 2020

tesuji commented Sep 27, 2020

nagisa commented Sep 27, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

rust-log-analyzer commented Sep 27, 2020

bors commented Sep 27, 2020

bjorn3 commented Sep 27, 2020 •

edited

Loading

rust-log-analyzer commented Sep 28, 2020

tesuji commented Sep 28, 2020

Use str::to_owned when format! has no formatting arguments #75894

Use str::to_owned when format! has no formatting arguments #75894

Conversation

tesuji commented Aug 25, 2020 • edited Loading

jyn514 commented Aug 25, 2020

rust-timer commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 25, 2020

rust-timer commented Aug 25, 2020

rust-timer commented Aug 25, 2020

tesuji commented Aug 25, 2020

tesuji commented Aug 26, 2020

nagisa left a comment

Choose a reason for hiding this comment

nagisa Aug 30, 2020

Choose a reason for hiding this comment

tesuji Aug 31, 2020

Choose a reason for hiding this comment

tesuji commented Aug 31, 2020

bors commented Sep 4, 2020

nagisa commented Sep 8, 2020

tesuji commented Sep 27, 2020

nagisa commented Sep 27, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

rust-log-analyzer commented Sep 27, 2020

bors commented Sep 27, 2020

bjorn3 commented Sep 27, 2020 • edited Loading

rust-log-analyzer commented Sep 28, 2020

tesuji commented Sep 28, 2020

Use `str::to_owned` when `format!` has no formatting arguments #75894

Use `str::to_owned` when `format!` has no formatting arguments #75894

tesuji commented Aug 25, 2020 •

edited

Loading

bjorn3 commented Sep 27, 2020 •

edited

Loading