Skip to content

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 4, 2017

Fixes #44240. Among other things, in crates that do a lot of formatting, this could reduce the number of items, although I haven't measured the performance benefits. If there's a codegen slowdown, that should IMO be solved by caching the output of miri, not by using static.

r? @alexcrichton

@alexcrichton
Copy link
Member

Just to confirm: you've verified that the codegen before and after for these callsites is the same? (these were carefully constructed to reduce code bloat)

@eddyb
Copy link
Member Author

eddyb commented Sep 5, 2017

@alexcrichton The static data in panic! and format_args! promote in MIR, but the LLVM IR is harder to diff - it looks like there is no difference in the number of allocas (so same stack usage), more static data and less runtime instructions (before optimizations, after both are identical).

@alexcrichton
Copy link
Member

Excellent!

@bors: r+

@bors
Copy link
Collaborator

bors commented Sep 5, 2017

📌 Commit 7e0b79d has been approved by alexcrichton

@arielb1 arielb1 added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 5, 2017
@alexcrichton
Copy link
Member

@bors: rollup

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 8, 2017
…lexcrichton

Use rvalue promotion to 'static instead of static items.

Fixes rust-lang#44240. Among other things, in crates that do a lot of formatting, this could reduce the number of items, although I haven't measured the performance benefits. If there's a codegen slowdown, that should IMO be solved by caching the output of miri, *not* by using `static`.

r? @alexcrichton
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 9, 2017
…lexcrichton

Use rvalue promotion to 'static instead of static items.

Fixes rust-lang#44240. Among other things, in crates that do a lot of formatting, this could reduce the number of items, although I haven't measured the performance benefits. If there's a codegen slowdown, that should IMO be solved by caching the output of miri, *not* by using `static`.

r? @alexcrichton
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 9, 2017
…lexcrichton

Use rvalue promotion to 'static instead of static items.

Fixes rust-lang#44240. Among other things, in crates that do a lot of formatting, this could reduce the number of items, although I haven't measured the performance benefits. If there's a codegen slowdown, that should IMO be solved by caching the output of miri, *not* by using `static`.

r? @alexcrichton
@aidanhs
Copy link
Member

aidanhs commented Sep 10, 2017

@bors r- rollup-

Suspect this caused failure in https://travis-ci.org/rust-lang/rust/jobs/273704890, rollup #44460:

[01:42:35]                   ((::fmt::format as
[01:42:35]                        fn(std::fmt::Arguments<'_>) -> std::string::String {std::fmt::format})(((<::std::fmt::Arguments>::new_v1
[01:42:35]                                                                                                    as
[01:42:35]                                                                                                    fn(&[&str], &[std::fmt::ArgumentV1<'_>]) -> std::fmt::Arguments<'_> {std::fmt::Arguments<'_>::new_v1})(({
[01:42:35]                                                                                                                                                                                                                static __STATIC_FMTSTR:
[01:42:35]                                                                                                                                                                                                                       &'static [&'static str]
[01:42:35]                                                                                                                                                                                                                       =
[01:42:35]                                                                                                                                                                                                                    (&([("test"
[01:42:35]                                                                                                                                                                                                                            as
[01:42:35]                                                                                                                                                                                                                            &'static str)]

vs

[01:42:35]                   ((::fmt::format as
[01:42:35]                        fn(std::fmt::Arguments<'_>) -> std::string::String {std::fmt::format})(((<::std::fmt::Arguments>::new_v1
[01:42:35]                                                                                                    as
[01:42:35]                                                                                                    fn(&[&str], &[std::fmt::ArgumentV1<'_>]) -> std::fmt::Arguments<'_> {std::fmt::Arguments<'_>::new_v1})((&([("test"
[01:42:35]                                                                                                                                                                                                                   as
[01:42:35]                                                                                                                                                                                                                   &'static str)]

@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2017

Wait, what, the PR builds don't check pretty tests? Ugh...

@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2017

Wow, I just ran ./x.py test for nothing... That explains why Travis didn't catch it on the PR though.

@eddyb eddyb force-pushed the static-by-any-other-name branch from 7e0b79d to 10f66bd Compare September 10, 2017 08:20
@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2017

@bors r=alexcrichton p=1 (let's not put it in a rollup, so we can get perf deltas)

@bors
Copy link
Collaborator

bors commented Sep 10, 2017

📌 Commit 10f66bd has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Sep 10, 2017

⌛ Testing commit 10f66bd with merge 23aaeb5...

bors added a commit that referenced this pull request Sep 10, 2017
Use rvalue promotion to 'static instead of static items.

Fixes #44240. Among other things, in crates that do a lot of formatting, this could reduce the number of items, although I haven't measured the performance benefits. If there's a codegen slowdown, that should IMO be solved by caching the output of miri, *not* by using `static`.

r? @alexcrichton
@bors
Copy link
Collaborator

bors commented Sep 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 23aaeb5 to master...

@bors bors merged commit 10f66bd into rust-lang:master Sep 10, 2017
@eddyb eddyb deleted the static-by-any-other-name branch September 10, 2017 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants