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

The optimize attribute #2412

Merged
merged 6 commits into from Oct 7, 2018

Conversation

@nagisa
Contributor

nagisa commented Apr 21, 2018

This is an RFC that has baked out after receiving feedback on the pre-RFC. Notably optimise(no) has been removed as it is not as important and seemed to have way more contention than optimise(size), while not being as useful or necessary as optimise(size).

Rendered
Tracking issue

@jonas-schievink

Nice to see progress on this!

---
Alternative: `optimize` (American English) instead of `optimise`… or both?

This comment has been minimized.

@jonas-schievink

jonas-schievink Apr 21, 2018

Contributor

Due to the precedence set by GCC and the fact that the vast majority of conversations I've read use american english spelling ("optimize", "optimizer") I'd prefer that over "optimise".

This comment has been minimized.

@Centril

Centril Apr 22, 2018

Contributor

For sure not both. As sad as this makes me (given that I prefer BrE), use of AmE is standard in the software industry, so we should stick with that.

This comment has been minimized.

@jesskfullwood

jesskfullwood May 1, 2018

At the risk of sounding pedantic (or worse, boring), I feel I should point out that the OED considers 'ize' to be perfectly acceptable and indeed preferred BrE spelling.

This comment has been minimized.

@Centril

Centril May 2, 2018

Contributor

@jesskfullwood You always learn something useful; and today is such a day :)

# Prior art
[prior-art]: #prior-art
* LLVM: `optsize`, `optnone`, `minsize` function attributes (exposed in Clang in some way);

This comment has been minimized.

@jonas-schievink

jonas-schievink Apr 21, 2018

Contributor

in some way

I think Clang supports __attribute__((optnone)) syntax for these

* LLVM: `optsize`, `optnone`, `minsize` function attributes (exposed in Clang in some way);
* GCC: `__attribute__((optimize))` function attribute which allows setting the optimisation level
and using certain(?) `-f` flags for each function;

This comment has been minimized.

@jonas-schievink

jonas-schievink Apr 21, 2018

Contributor

Per-function optimization can also be configured by using #pragma GCC optimize (...).

(see https://gcc.gnu.org/onlinedocs/gcc/Function-Specific-Option-Pragmas.html)

@Centril Centril added the T-lang label Apr 22, 2018

# Unresolved questions
[unresolved]: #unresolved-questions
* Should we support such an attribute at module-level? Crate-level?

This comment has been minimized.

@Centril

Centril Apr 22, 2018

Contributor

I think so yes. At least, you should be able to specify #[optimize(size)] on mod and impl with the semantics that it applies to every fn inside those transitively.

This comment has been minimized.

@clarcharr

clarcharr Apr 26, 2018

Contributor

Counter point: people might assume that optimise(size) at the crate level also asserts repr(packed). We don't do this with inline so I'd assume not for optimise.

This comment has been minimized.

@Centril

Centril May 24, 2018

Contributor

I think it would be quite tedious to have to do this on every single function if a module if that is what you want. If we want to be more clear that it is about functions, you could write #[optimize(fn_size)] mod foobar { .. }.

This comment has been minimized.

@joshtriplett

joshtriplett Jun 25, 2018

Member

I do think it makes sense to allow #![optimize(size)] in a crate to apply to all functions (that don't override it).

I certainly don't think optimize(size) should ever imply repr(packed), because the latter can affect semantics.

```rust
#[optimise(size)]
fn banana() {

This comment was marked as resolved.

@Centril

Centril Apr 22, 2018

Contributor

This function is monomorphic. How do you deal with polymorphic functions?

This comment was marked as resolved.

@nagisa

nagisa Apr 22, 2018

Contributor

Where attributes are involved, monomorphisations behave as if they were monomorphic functions annotated with the same attributes as the generic function they were monomorphised from.

This is generally true, so I didn’t see too much reason to explicitly note this in RFC.

This comment was marked as resolved.

@Centril

Centril Apr 22, 2018

Contributor

That sounds good to me. 👍

@Havvy

This comment has been minimized.

Contributor

Havvy commented Apr 24, 2018

Two questions.

  1. If I apply this attribute to a function that creates a closure, does that closure also get optimized for size?
  2. What happens when I apply this attribute to a non-function, such as a struct.
@rkruppe

This comment has been minimized.

Contributor

rkruppe commented Apr 25, 2018

If I apply this attribute to a function that creates a closure, does that closure also get optimized for size?

Interesting question. Precedent from inline would be "no" (closures are always inline and not inline(never) regardless of where they are) with the possibility of adding the attribute to the closure expression as well (e.g. #[inline(never)] #[optimize(size)] |x| x + 1). On the other hand, especially if the attribute can be applied to modules, it is very reasonable to expect it to be passed down.

What happens when I apply this attribute to a non-function, such as a struct.

Should be an error. (With the possible exception of closure expressions.)

@nagisa

This comment has been minimized.

Contributor

nagisa commented Apr 25, 2018

@rkruppe

This comment has been minimized.

Contributor

rkruppe commented Apr 25, 2018

Some misuses of known attributes are hard errors (example). Others aren't, but AFAIK that's mostly for backwards compatibility because rust 1.0 wasn't very comprehensive at identifying all such attributes.

@retep998

This comment has been minimized.

Member

retep998 commented Apr 25, 2018

I insist on the American optimize over optimise.

@kornelski

This comment has been minimized.

Contributor

kornelski commented Apr 26, 2018

Most of the functions will be "cold", so I think it would make more sense to use -C opt-level=s to make all of them small, and then add #[optimize(speed)] on the few hot functions.

@nagisa

This comment has been minimized.

Contributor

nagisa commented Apr 26, 2018

@nikomatsakis nikomatsakis self-assigned this Apr 26, 2018

@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Apr 26, 2018

@nagisa some attributes error when used in the wrong position, and I'd say precedent is that this should error when used on a non-function for now.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 24, 2018

@rfcbot fcp merge

It seems like conversation here has reached a fixed point. I personally think exposing these sorts of knobs is a good idea — naturally they should come with no firm promises. I'd like to hear from some members of the Embedded Domain WG (cc @japaric) but I'm going to assume they are in favor. =)

@rfcbot

This comment has been minimized.

rfcbot commented May 24, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented May 24, 2018

It might be that this should be more of a @rust-lang/compiler RFC? In any case, cc compiler folks — take a look.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented May 24, 2018

Does it really make sense to globally optimize a program and then tag specific functions as size-optimized? I'd think for the embedded use case, you'd want the opposite: optimize the whole program for size and then tag a few hotspots as performance-optimized.

I don't have a fundamental objection to this, but I'm curious to what degree it reflects "people actively want to use this right now" versus "people kinda think unspecified other people might want a thing kinda like this".

@cramertj

This comment has been minimized.

Member

cramertj commented May 24, 2018

@joshtriplett I'd definitely appreciate something like this and would use it if it were available, but i agree with you that I think what I really want is the ability to have opt-level=z by default, and opt into performance improvements in areas that need it. Note that @kornelski and @nagisa discussed this above, and it doesn't sound like it's necessarily feasible.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented May 24, 2018

@cramertj That was my expectation as well. Even despite the lack of global optimization, the ability to optimize specific functions for speed ought to help address hotspots found via profiling (with subsequent profiling after adding the attributes to see if it worked).

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Jun 14, 2018

I've been thinking about this RFC for a while, ever since it was proposed for FCP, and debating whether to check the box for it. After careful discussion:

@rfcbot concern optimize-speed

Most programs needing functionality like this will want global size optimization and selective optimize(speed). It seems exceedingly unlikely that a program will want to globally optimize for performance and selectively optimize a few specific functions for size. (I can imagine very unusual scenarios that might want to use that, but it seems like the far less common case.)

I acknowledge the point that optimize(speed) might make global optimizations harder. However, it would still allow function-local optimizations, and some limited degree of global optimizations. Making optimize(speed) more useful would be the domain of the compiler, and I can imagine people making future improvements or feature requests along the lines of "I used optimize(speed) on this function and I expected the compiler to make this optimization but it didn't". Those are implementation details, not blockers for the concept of optimize(speed).

Based on that, I'd like to propose revising the RFC to define both optimize(speed) and optimize(size).

@nagisa

This comment has been minimized.

Contributor

nagisa commented Jun 21, 2018

I thought about optimise(speed) and I think it could be implemented, but I’m not sure if the implementation approach would be equivalent to the -Copt-level flags, still.

The approach would basically involve always compiling with what is now -Copt-level=2/3 and for -Copt-level=s/z adding the optimise(size) attribute to all functions not annotated with optimise(speed).

@nagisa

This comment has been minimized.

Contributor

nagisa commented Jun 21, 2018

If we decide to go towards optimise(speed) we have to decide what level of opt-level that would refer to, because optimise(size=2) and optimise(size=3) is definitely not something that you could implement in the LLVM backend.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Jun 25, 2018

@nagisa

If we decide to go towards optimise(speed) we have to decide what level of opt-level that would refer to...

This seems like something that users might want to select (e.g., by specifying -Copt-level=s3 or something). For now I think it'd be reasonable to pick an default and reserve the right to tweak it -- I'd go with whatever level of optimization cargo build --release uses by default. In any case, this feels like something that the RFC can plausibly kick down the road as an unresolved question to me(i.e., what mechanism, if any, should we offer to let users override the default?).

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Jun 25, 2018

I like the idea of having optimize(speed) default to the standard optimization level used by release builds, and then letting users specify the speed optimization level separately if desired.

At the risk of bikeshedding: rather than -Copt-level=s3, how about -Copt-level=z -Copt-level-speed=3? (Likewise for Cargo profile options.)

# Summary
[summary]: #summary
This RFC introduces the `#[optimize]` attribute for controlling optimisation level on a per-item

This comment has been minimized.

@joshtriplett

joshtriplett Jul 13, 2018

Member

This is a massive nit, but given that the attribute is spelled optimize for consistency with precedent and expectations from other tools and environments, could you please use that same spelling consistently in the RFC? And, in particular, in the filename (currently 0000-optimise-attr.md)?

Also, given that it seems likely to become an issue, could you explicitly mention, somewhere in the RFC, that the spelling "optimize" was chosen specifically based on precedent and expectations from other tools and environments, and that the implementation should include a test to confirm that optimise produces a clear error message that points people to the correct spelling (including a rustfix)?

This comment has been minimized.

@nagisa

nagisa Jul 13, 2018

Contributor

I’ll try to fix spelling before a merge.

implementation should include a test

I feel that it would be sufficient to require such a test in the tracking issue.

could you explicitly mention, somewhere in the RFC

Seems like a difficult thing to fit into the RFC without it sticking out like a sore thumb. Not sure if it is necessary either, given that -ize spelling is prefered across the compiler anyway.

This comment has been minimized.

@joshtriplett

joshtriplett Jul 13, 2018

Member

You already have an "Rationale and alternatives" section; that seems like the right place for a one-paragraph mention.

I'm mostly mentioning it here so it doesn't get lost. I wouldn't be surprised if Rust's existing spelling-correction mechanisms catch this, which just makes it a matter of a single test case.

In any case, this is a nit and I'm not particularly worried about it.

This comment has been minimized.

@Centril

Centril Jul 13, 2018

Contributor

Do we have more attributes of the form -ize? If so, we could do a more general version of the error message, or just employ the usual levenshtein distance on attribute names...

This comment has been minimized.

@nagisa

nagisa Jul 13, 2018

Contributor

-ize in general, not just attributes, but even in attributes there’s not a single hit for -ise, but a few for -ize:

Multiple CLI flags use -ize spelling as well.

@nagisa

This comment has been minimized.

Contributor

nagisa commented Sep 24, 2018

@pnkfelix @scottmcm @withoutboats It has been 4 months since the pre-FCP has begun, and your checkboxes are still unchecked without any outstanding concerns from either of you.

@rfcbot

This comment has been minimized.

rfcbot commented Sep 24, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 24, 2018

@nagisa I see no mention of the treatment of closures in the RFC but it has been discussed in comments without a resolution. Could you please record an unresolved question, in the text, to consider before stabilizing?

EDIT: could you also clarify which unresolved questions are to be resolved during the evaluation/stabilization process and which would need subsequent RFCs?

Change the wording to accomodate expressions
Additionally, clarify propagation of the attribute.
@nagisa

This comment has been minimized.

Contributor

nagisa commented Sep 25, 2018

I adjusted wording of the RFC to consider closures. Also clarified wording around propagation somewhat which should answer everything /wrt closures. My feeling is that these modifications are sufficiently simple and an obvious extension of the previous wording, but just to be safe, I also added an item in unresolved questions to make sure it ends up in the tracking issue.

could you also clarify which unresolved questions are to be resolved during the evaluation/stabilization process and which would need subsequent RFCs?

I don’t think any of the unresolved questions will need to be discussed for stabilisation, except for the one I added in the most recent commit. They were intended to be more a question of whether we care about those use cases enough for this RFC, rather than that they are outright unresolved.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 25, 2018

@nagisa That also works for me :)

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 25, 2018

PS: If you want to care of the nits in #2412 (comment) I can deal with the merge procedure once the FCP is over.

@vi

This comment has been minimized.

vi commented Sep 26, 2018

"Rendered" link is 404.

s to z

Would there be an auto-suggestion if mistyped with s?

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 26, 2018

"Rendered" link is 404.

Fixed. :)

@nagisa

This comment has been minimized.

Contributor

nagisa commented Sep 26, 2018

Would there be an auto-suggestion if mistyped with s?

A better place for this would be some functionality common to attributes in general (similar to how we suggest similar names when variables are misspelled), but that would be another RFC altogether. For now you’d just get an unused attribute warning, I guess.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 26, 2018

@nagisa might just be a pure diagnostics issue (e.g. just apply levenshtein) in which case it doesn't need an RFC and the compiler team can just do it?

@nagisa

This comment has been minimized.

Contributor

nagisa commented Sep 26, 2018

@Centril attributes have more subtlety compared to local variables. This is especially notable when syntax plugins can introduce arbitrary attribute names into a program.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 26, 2018

@nagisa right; but shouldn't the set of attribute names be in scope such that when you try to apply an attribute, if resolution does not find it in the attr-macro sub-namespace then an error is emitted in which the compiler checks which paths look similar (using the familiar similarity algorithms...)? E.g. shouldn't it be similar to use foo::bar::bez; which doesn't exist because I should have written baz instead?

@nagisa

This comment has been minimized.

Contributor

nagisa commented Sep 26, 2018

@Centril There is no scope for attribute names, they are all global AFAIK. Yet, attributes generally may only be used with certain logical constructs (e.g. optimize can only be used with function-like things). You don’t want to levenstein-suggest to change #[boline] into #[inline] on top of a struct, when there’s also plugin-introduced #[boolinate] which is applicable in that location :)

This is probably not the right place to discuss such a feature in depth. As far as the original question is concerned, I feel that the preexisting unused_attribute lint is an appropriate solution for the scope of this RFC. I’d love something more directed, but I’m of an opinion that such solution would be better off developed separately and in the context of attributes in general, not this specific attribute.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 26, 2018

@jrpascucci

This comment has been minimized.

jrpascucci commented Oct 4, 2018

@nagisa This may be too late in the process to bring it up, and maybe there's a subtlety I've missed, but in the pre-RFC thread you said "...I don’t think optimise(none) is applicable for the crypto use-case."

The original questioner is correct about side-channel attacks (last rfc I could find about this kind of thing was this one. A particular case would be in implementing countermeasures against timing attacks) Many of these involve doing a bunch of things that a medium smart compiler of any language could trivially detect are not useful to the result and could be thrown away.

I've come across one or two crates that use different contortions to confuse the compiler into avoiding some optimization, and one could recourse to something unsafe, but even sneaky ways may be amenable to optimization and thereafter attack. As a practical instance, it's not clear to me that this crate guaranteeably works now or will in the future. I, for one, would be more confident in it if it had an optimize(none).

Being able to disable optimization of a particular routine as far as one can (which should get one to the same level as debug execution, I would think) should help.

@rfcbot

This comment has been minimized.

rfcbot commented Oct 4, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@nagisa

This comment has been minimized.

Contributor

nagisa commented Oct 4, 2018

@jrpascucci you want something that would guarantee the properties you seek of your code and as far as I know the only way to get them is to write assembly, optimize(none) does not guarantee anything interesting. If there’s a constant-time operation implemented in Rust or C, that code is not to be trusted by default regardless of the flags on top of the function.

@Centril Centril referenced this pull request Oct 7, 2018

Open

Tracking issue for RFC 2412, "The optimize attribute" #54882

2 of 8 tasks complete

@Centril Centril merged commit 4baa3fc into rust-lang:master Oct 7, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Oct 7, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#54882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment