Explain why inlining default ToString impl #74852

tesuji · 2020-07-28T00:54:25Z

Trying to remove inline attribute from default ToString impl causes regression.
Perf result at #74852 (comment).

tesuji · 2020-07-28T01:01:16Z

Regress in wall-time of debug and check build?

tesuji · 2020-07-28T02:26:19Z

Can I have a perf run, @Mark-Simulacrum ?

Mark-Simulacrum · 2020-07-28T02:27:24Z

@bors try @rust-timer queue

rust-timer · 2020-07-28T02:27:25Z

Awaiting bors try build completion

bors · 2020-07-28T02:27:36Z

⌛ Trying commit 682726ecab5eb6e0c7b6f4cf5698360fb9fb3300 with merge afd3e8ab7e3e1c925fc3893aab125617395808c8...

bors · 2020-07-28T03:07:55Z

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

rust-timer · 2020-07-28T03:07:57Z

Queued afd3e8ab7e3e1c925fc3893aab125617395808c8 with parent 9be8ffc, future comparison URL.

rust-timer · 2020-07-28T06:50:26Z

Finished benchmarking try commit (afd3e8ab7e3e1c925fc3893aab125617395808c8): 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

tesuji · 2020-07-28T09:07:13Z

The number is ... different than local perf.
Probably because:

I used stage1 compiler
I used incremental build locally.
The machine I used to perf is different:
- Intel(R) Xeon(R) CPU E5-2660 0 @ 2.20GHz
- 2 sockets, 16 cores, 32 threads
- RAM: 144862 MB
- HDD Drive.

I will try to perf with non-incremental and compare.

tesuji · 2020-07-28T13:47:20Z

I tried 2 approaches with master commit 1f5d69d:

Remove inline attribute from default impl ToString for T: fmt::Display + ?Sized.
Remove all inline attributes from all impl ToString in library/alloc/src/string.rs.

The locally perf result is here: results.db.tar.gz.
Copy it to locally checkout of https://github.com/rust-lang/rustc-perf and run cargo build --release --bin=site ./result.db
to view the result.
There are 3 IDs: master, default-tostring-rm-inline, rm-inline-all.

The perf result is complicated, most are regressions. But I think it is better to be decided by perf experts.

In the case removing inline attribute is unaccepted, I would like to add a comment in the code to explain why.

nnethercote · 2020-07-28T22:27:29Z

I looked at the results, they are clearly regressions. I don't think comments are necessary, because it's not that surprising that marking a hot function as inline would improve performance, and doing the opposite would hurt performance. But if you really want to add comments I won't object.

tesuji · 2020-07-29T03:15:50Z

Updated PR description.

nnethercote · 2020-07-29T05:44:16Z

library/alloc/src/string.rs

@@ -2196,6 +2196,9 @@ pub trait ToString {
 /// since `fmt::Write for String` never returns an error itself.
 #[stable(feature = "rust1", since = "1.0.0")]
 impl<T: fmt::Display + ?Sized> ToString for T {
+    // The general guide line is to not inline generic functions. However,


s/guide line/guideline/

Is that really a guideline? I'm sure I've seen many generic functions marked with inline.

The guideline is documented here: https://forge.rust-lang.org/libs/maintaining-std.html#is-that-inline-right .
Or I may infer it wrong.

nnethercote · 2020-07-29T05:44:40Z

r=me once the typo is fixed.

nnethercote · 2020-07-29T07:51:00Z

@bors r+

bors · 2020-07-29T07:51:01Z

📌 Commit 27e1b06 has been approved by nnethercote

nnethercote · 2020-07-29T07:51:07Z

@bors rollup=always

bors · 2020-07-29T07:51:11Z

⌛ Testing commit 27e1b06 with merge 78c154293cdf2aafd42d7b65ecaafb689e996d1c...

bors · 2020-07-29T08:20:37Z

💔 Test failed - checks-actions

tesuji · 2020-07-29T08:39:37Z

Spurious error

thread 'lto::dev_profile' panicked at '
Expected: execs
    but: differences:
  6 - |[RUNNING] `rustc --crate-name foo [..]--crate-type lib --emit=dep-info,metadata,link -Clinker-plugin-lto [..]|
    + |     Running `rustc --crate-name foo --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --emit=dep-info,link -Cembed-bitcode=no -C debuginfo=2 --test -C metadata=110e52c96c1b8618 -C extra-filename=-110e52c96c1b8618 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t1067/foo/target/debug/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t1067/foo/target/debug/deps --extern bar=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t1067/foo/target/debug/deps/libbar-d2876efba7515ca8.rlib`|

  7 - |[RUNNING] `rustc --crate-name foo [..]--emit=dep-info,link -Cembed-bitcode=no [..]--test[..]|
    + |     Running `rustc --crate-name foo --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -Clinker-plugin-lto -C debuginfo=2 -C metadata=aae1986730f9b7d9 -C extra-filename=-aae1986730f9b7d9 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t1067/foo/target/debug/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t1067/foo/target/debug/deps --extern bar=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t1067/foo/target/debug/deps/libbar-d2876efba7515ca8.rmeta`|

nnethercote · 2020-07-29T19:19:50Z

@bors r+

bors · 2020-07-29T19:19:51Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Upgrade to LLVM 11 (rc2) #73526

bors · 2020-07-29T19:19:52Z

📌 Commit 27e1b06 has been approved by nnethercote

@ghost

…arth Rollup of 10 pull requests Successful merges: - rust-lang#74742 (Remove links to rejected errata 4406 for RFC 4291) - rust-lang#74819 (Point towards `format_spec`; it is in other direction) - rust-lang#74852 (Explain why inlining default ToString impl) - rust-lang#74869 (Make closures and generators a must use types) - rust-lang#74873 (symbol mangling: use ty::print::Print for consts) - rust-lang#74902 (Remove deprecated unstable `{Box,Rc,Arc}::into_raw_non_null` functions) - rust-lang#74904 (Fix some typos in src/librustdoc/clean/auto_trait.rs) - rust-lang#74910 (fence docs: fix example Mutex) - rust-lang#74912 (Fix broken link in unstable book `plugin`) - rust-lang#74927 (Change the target data layout to specify more values) Failed merges: r? @ghost

rust-highfive assigned nnethercote Jul 28, 2020

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

tesuji force-pushed the inline-rm-tostring branch from 682726e to 7683f68 Compare July 28, 2020 08:56

tesuji force-pushed the inline-rm-tostring branch from 7683f68 to b91de8c Compare July 29, 2020 03:11

tesuji changed the title ~~Remove inline from default ToString impl~~ Explain why inlining default ToString impl Jul 29, 2020

nnethercote reviewed Jul 29, 2020

View reviewed changes

Explain why inline default ToString impl

27e1b06

tesuji force-pushed the inline-rm-tostring branch from b91de8c to 27e1b06 Compare July 29, 2020 07:38

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 Jul 29, 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 Jul 29, 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 Jul 29, 2020

Manishearth mentioned this pull request Jul 29, 2020

Rollup of 10 pull requests #74929

Merged

bors merged commit c07998e into rust-lang:master Jul 30, 2020

tesuji deleted the inline-rm-tostring branch July 30, 2020 02:09

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain why inlining default ToString impl #74852

Explain why inlining default ToString impl #74852

tesuji commented Jul 28, 2020 •

edited

Loading

tesuji commented Jul 28, 2020 •

edited

Loading

tesuji commented Jul 28, 2020

Mark-Simulacrum commented Jul 28, 2020

rust-timer commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

rust-timer commented Jul 28, 2020

rust-timer commented Jul 28, 2020

tesuji commented Jul 28, 2020 •

edited

Loading

tesuji commented Jul 28, 2020

nnethercote commented Jul 28, 2020

tesuji commented Jul 29, 2020

nnethercote Jul 29, 2020

tesuji Jul 29, 2020

nnethercote commented Jul 29, 2020

nnethercote commented Jul 29, 2020

bors commented Jul 29, 2020

nnethercote commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

tesuji commented Jul 29, 2020

nnethercote commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

Explain why inlining default ToString impl #74852

Explain why inlining default ToString impl #74852

Conversation

tesuji commented Jul 28, 2020 • edited Loading

tesuji commented Jul 28, 2020 • edited Loading

tesuji commented Jul 28, 2020

Mark-Simulacrum commented Jul 28, 2020

rust-timer commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

rust-timer commented Jul 28, 2020

rust-timer commented Jul 28, 2020

tesuji commented Jul 28, 2020 • edited Loading

tesuji commented Jul 28, 2020

nnethercote commented Jul 28, 2020

tesuji commented Jul 29, 2020

nnethercote Jul 29, 2020

Choose a reason for hiding this comment

tesuji Jul 29, 2020

Choose a reason for hiding this comment

nnethercote commented Jul 29, 2020

nnethercote commented Jul 29, 2020

bors commented Jul 29, 2020

nnethercote commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

tesuji commented Jul 29, 2020

nnethercote commented Jul 29, 2020

bors commented Jul 29, 2020

bors commented Jul 29, 2020

tesuji commented Jul 28, 2020 •

edited

Loading

tesuji commented Jul 28, 2020 •

edited

Loading

tesuji commented Jul 28, 2020 •

edited

Loading