Skip to content
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

Implement optimization fuel and re-enable struct field reordering #40377

Merged
merged 9 commits into from
Apr 12, 2017

Conversation

ahicks92
Copy link
Contributor

@ahicks92 ahicks92 commented Mar 9, 2017

See this discussion for background.

This pull request adds two new compilation options: -Z print-fuel=crate prints the optimization fuel used by a crate and -Z fuel=crate=n sets the optimization fuel for a crate.

It also turns field reordering back on. There is no way to test this feature without something consuming fuel. We can roll this back if we want, but then the optimization fuel bits will be dead code.

The one notable absence from this PR is a test case. I'm not sure how to do one that's worth having. The only thing I can think of to test is -Z fuel=foo=0. The problem with other tests is that either (1) they're so big that future optimizations will apply, thus breaking them or (2) we don't know which order the optimizations will be applied in, so we can't guess the message that will be printed. If someone has a useful proposal for a good test, I certainly want to add one.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@ahicks92
Copy link
Contributor Author

ahicks92 commented Mar 9, 2017

To be honest, we probably want:

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned arielb1 Mar 9, 2017
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except for the (accidental) submodule changes.

@ahicks92 ahicks92 force-pushed the optimization_fuel branch from 8bd1709 to de905e3 Compare March 9, 2017 02:57
@@ -504,6 +518,33 @@ impl Session {
println!("Total time spent decoding DefPath tables: {}",
duration_to_secs_str(self.perf_stats.decode_def_path_tables_time.get()));
}

/// We want to know if we're allowed to do an optimization for crate crate.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "crate crate" intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly I'm going to go with yes, though crate foo might be a better choice for this phrasing. It's something I would write, though, with the intent that the redundant crate is a variable.

@eddyb
Copy link
Member

eddyb commented Mar 9, 2017

I was going to cc @rust-lang/compiler, but I think I'll also nominate it for discussion tomorrow.

@eddyb eddyb added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 9, 2017
// The odd pattern here avoids a warning about the value never being read.
if can_optimize { can_optimize = false; }
let can_optimize = (fields.len() > 2 || StructKind::EnumVariant == kind)
&& ! (repr.c || repr.packed || repr.linear || repr.simd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a change in behavior (and though seemingly correct, I want to make sure): before we didn't optimize for repr(c) and repr(packed), now repr(linear) and repr(simd) are presumably added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't optimize for repr(simd) before. There might be a case for removing it from the condition. Simd vectors (used to?) go through Layout::Vector. I'm not as familiar with the details as I used to be: I had to leave this for a month in the middle. But I think leaving it for clarity is fine.

repr(linear) is not a thing externally. It exists because we need to be able to remember which structs reordering was disabled for when using optimization fuel. It is so named because there has been talk of making an RFC that adds it, and at this point it would just be exposing it via the parser.

Copy link
Member

@eddyb eddyb Mar 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout::Vector is still unchanged, so repr.simd should never be true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to kill it? I can, but it seems like the kind of thing where we might get rid of Layout::Vector and get mysterious breakage and I don't think it's doing any harm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah it's fine like this.

@nikomatsakis
Copy link
Contributor

Discussed in @rust-lang/compiler -- let's land this thing! It'd be nice to have some sort of tests. We talked about having a test that checks that we disabled reordering on one out of two structs (by supplying one unit of fuel). We might have to adjust the test in the future but for now it's ok.

@ahicks92
Copy link
Contributor Author

ahicks92 commented Mar 9, 2017

I hope to do the tests this evening when I get back. Should they be under run-pass? Going to hit the no fuel and 1 fuel cases.

@eddyb
Copy link
Member

eddyb commented Mar 9, 2017

@camlorn Maybe an ui test for the layout of the structs? i.e. like the existing ones you updated here.

@ahicks92
Copy link
Contributor Author

ahicks92 commented Mar 9, 2017

@eddyb
that would require knowing which one is being optimized. @nikomatsakis's idea is better: get the size of 2 structs and assert that only one of them is right.

But this means it won't have a UI.

@eddyb
Copy link
Member

eddyb commented Mar 9, 2017

@camlorn It's in AST order, i.e. struct Foo; struct Bar; will need 1 for the first and 2 for both.
That's ignoring on-demand but on-demand should only kick in if you mention that type elsewhere.

@ahicks92
Copy link
Contributor Author

@eddyb
I will do a UI test if that is what you really really want. But it's kind of overkill. All we have to do is ask if the size is right, because a bunch of other tests already cover the optimization itself.

I can't get to this tonight, it's going to have to be the morning. It will not be some morning in a month from now, it will be tomorrow morning.

@ahicks92
Copy link
Contributor Author

Okay. There we go. 100% test coverage. And I don't think any of them are even fragile.

@eddyb
Copy link
Member

eddyb commented Mar 10, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Mar 10, 2017

📌 Commit 94a0375 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Mar 13, 2017

🔒 Merge conflict

@eddyb
Copy link
Member

eddyb commented Mar 13, 2017

@bors retry

@bors
Copy link
Contributor

bors commented Mar 13, 2017

🔒 Merge conflict

@alexcrichton
Copy link
Member

@bors: r=eddyb

@bors
Copy link
Contributor

bors commented Mar 13, 2017

📌 Commit 3fb94b7 has been approved by eddyb

@ahicks92
Copy link
Contributor Author

Wait, what happened here?

@alexcrichton
Copy link
Member

bors is getting confused nowadays due to submodule updates and such, so it'll think there's a merge conflict when there isn't one. I rebased the branch and r+'d it.

@bors
Copy link
Contributor

bors commented Apr 11, 2017

💔 Test failed - status-travis

@TimNN
Copy link
Contributor

TimNN commented Apr 11, 2017

@ahicks92
Copy link
Contributor Author

@eddyb
Just to repeat what got said on IRC for anyone else following, we did fix stuff related to this already.

No one is really sure how in the world the compiler passed all the tests in January at this point, but, well, it did.

@eddyb
Copy link
Member

eddyb commented Apr 11, 2017

I figured out what was happening: in rustc_trans we used to have a few shims that proxied their arguments irrespective of their types - that means that extern "rust-call" fn shims (e.g. Fn trait objects) only saw the expanded tuples, and never had a value of the tuple type.

However, @arielb1's #39628 resulted in the shims in those cases (e.g. again, Fn trait object) being translated the same way as functions defined extern "rust-call" fn, which involved the incorrect code storing arguments to tuple fields, never have been fixed until today.

The only reason it didn't break before is because such functions are rare and require unstable Rust to write - the only semi-common example I can think of is FnBox which isn't usually used with multiple arguments.

@bors
Copy link
Contributor

bors commented Apr 11, 2017

⌛ Testing commit e18c59f with merge 9aacb72...

@frewsxcv
Copy link
Member

@bors retry

prioritizing #41231 since everything in the queue will fail without it

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 11, 2017
Implement optimization fuel and re-enable struct field reordering

See [this discussion](https://internals.rust-lang.org/t/rolling-out-or-unrolling-struct-field-reorderings/4485) for background.

This pull request adds two new compilation options: `-Z print-fuel=crate` prints the optimization fuel used by a crate and `-Z fuel=crate=n` sets the optimization fuel for a crate.

It also turns field reordering back on.  There is no way to test this feature without something consuming fuel.  We can roll this back if we want, but then the optimization fuel bits will be dead code.

The one notable absence from this PR is a test case.  I'm not sure how to do one that's worth having.  The only thing I can think of to test is `-Z fuel=foo=0`.  The problem with other tests is that either (1) they're so big that future optimizations will apply, thus breaking them or (2) we don't know which order the optimizations will be applied in, so we can't guess the message that will be printed.  If someone has a useful proposal for a good test, I certainly want to add one.
bors added a commit that referenced this pull request Apr 12, 2017
Rollup of 8 pull requests

- Successful merges: #40377, #40559, #41173, #41202, #41204, #41209, #41216, #41231
- Failed merges:
@bors
Copy link
Contributor

bors commented Apr 12, 2017

⌛ Testing commit e18c59f with merge da32752...

@bors bors merged commit e18c59f into rust-lang:master Apr 12, 2017
SimonSapin added a commit to servo/servo that referenced this pull request Apr 15, 2017
This version enables [struct field reordering][1] which brings the size
of the types for specified values of some CSS properties under the threshold
such that they shouldn’t be boxed anymore, making test unit fail.

Simly unboxing them moves the test failure to Stylo’s unit test,
since the stable compiler used in that case does not do field re-ordering.
Therefore, we manually reorder a couple fields to effectively bring this
optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377
SimonSapin added a commit to servo/servo that referenced this pull request Apr 15, 2017
This version enables [struct field reordering][1] which brings the size
of the types for specified values of some CSS properties under the threshold
such that they shouldn’t be boxed anymore, making test unit fail.

Simply unboxing them moves the test failure to Stylo’s unit test,
since the stable compiler used in that case does not do field re-ordering.
Therefore, we manually reorder a couple fields to effectively bring this
optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377
SimonSapin added a commit to servo/servo that referenced this pull request Apr 15, 2017
This version enables [struct field reordering][1] which brings the size
of the types for specified values of some CSS properties under the threshold
such that they shouldn’t be boxed anymore, making test unit fail.

Simply unboxing them moves the test failure to Stylo’s unit tests,
since the stable compiler used in that case does not do field re-ordering.
Therefore, we manually reorder a couple fields to effectively bring this
optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377
SimonSapin added a commit to servo/servo that referenced this pull request Apr 15, 2017
This version enables [struct field reordering][1] which brings the size
of the types for specified values of some CSS properties under the threshold
such that they shouldn’t be boxed anymore, making test unit fail.

Simply unboxing them moves the test failure to Stylo’s unit tests,
since the stable compiler used in that case does not do field re-ordering.
Therefore, we manually reorder a couple fields to effectively bring this
optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377
SimonSapin added a commit to servo/servo that referenced this pull request Apr 15, 2017
This version enables [struct field reordering][1] which brings the size
of the types for specified values of some CSS properties under the threshold
such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests,
since the stable compiler used in that case does not do field re-ordering.
Therefore, we manually reorder a couple fields to effectively bring this
optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377
bors-servo pushed a commit to servo/servo that referenced this pull request Apr 16, 2017
Upgrade to rustc 1.18.0-nightly (5f13a3b54 2017-04-15)

This version enables [struct field reordering][1] which brings the size of the types for specified values of some CSS properties under the threshold such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests, since the stable compiler used in that case does not do field re-ordering. Therefore, we manually reorder a couple fields to effectively bring this optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16473)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 16, 2017
…-04-15) (from servo:rustup); r=emilio

This version enables [struct field reordering][1] which brings the size of the types for specified values of some CSS properties under the threshold such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests, since the stable compiler used in that case does not do field re-ordering. Therefore, we manually reorder a couple fields to effectively bring this optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377

Source-Repo: https://github.com/servo/servo
Source-Revision: c453e2ef89b32798dabbb23b22cfd5a72dddf6a5

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 89b15d84b3f4b106d58ca90b3fd18f552d1a6633
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Apr 21, 2017
…-04-15) (from servo:rustup); r=emilio

This version enables [struct field reordering][1] which brings the size of the types for specified values of some CSS properties under the threshold such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests, since the stable compiler used in that case does not do field re-ordering. Therefore, we manually reorder a couple fields to effectively bring this optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377

Source-Repo: https://github.com/servo/servo
Source-Revision: c453e2ef89b32798dabbb23b22cfd5a72dddf6a5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…-04-15) (from servo:rustup); r=emilio

This version enables [struct field reordering][1] which brings the size of the types for specified values of some CSS properties under the threshold such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests, since the stable compiler used in that case does not do field re-ordering. Therefore, we manually reorder a couple fields to effectively bring this optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377

Source-Repo: https://github.com/servo/servo
Source-Revision: c453e2ef89b32798dabbb23b22cfd5a72dddf6a5

UltraBlame original commit: 55519369af2b2a65f2a5c2275522a8b1dc014953
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…-04-15) (from servo:rustup); r=emilio

This version enables [struct field reordering][1] which brings the size of the types for specified values of some CSS properties under the threshold such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests, since the stable compiler used in that case does not do field re-ordering. Therefore, we manually reorder a couple fields to effectively bring this optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377

Source-Repo: https://github.com/servo/servo
Source-Revision: c453e2ef89b32798dabbb23b22cfd5a72dddf6a5

UltraBlame original commit: 55519369af2b2a65f2a5c2275522a8b1dc014953
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…-04-15) (from servo:rustup); r=emilio

This version enables [struct field reordering][1] which brings the size of the types for specified values of some CSS properties under the threshold such that they shouldn’t be boxed anymore, making unit tests fail.

Simply unboxing them moves the test failure to Stylo’s unit tests, since the stable compiler used in that case does not do field re-ordering. Therefore, we manually reorder a couple fields to effectively bring this optimization to older compilers for a few specific types.

[1]: rust-lang/rust#40377

Source-Repo: https://github.com/servo/servo
Source-Revision: c453e2ef89b32798dabbb23b22cfd5a72dddf6a5

UltraBlame original commit: 55519369af2b2a65f2a5c2275522a8b1dc014953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.