[API review] Add MethodImplOptions.AggressiveOptimization

@noahfalk

Add MethodImplOptions.AggressiveOptimization as a counterpart to MethodImplOptions.NoOptimization. NoOptimization would override AggressiveOptimization.

    [Flags]
    public enum MethodImplOptions
    {
        // Unmanaged = 0x0004,
        // NoInlining = 0x0008,
        // ForwardRef = 0x0010,
        // Synchronized = 0x0020,
        // NoOptimization = 0x0040,
        // PreserveSig = 0x0080,
        // AggressiveInlining = 0x0100,
        AggressiveOptimization = 0x0200,
        // InternalCall = 0x1000
    }

This would also add a new keyword aggressiveoptimization to IL ASM:

  .method private hidebysig static void  Main() cil managed aggressiveoptimization

The flag could be used in a MethodImplAttribute to indicate that the method contains hot code:

For tiering, it could cause tier 1 JIT to be used right away for the method, as a workaround for https://github.com/dotnet/coreclr/issues/19751
- For this reason, we would also like to add this API to 2.2, as it is currently planned to enable tiering by default in 2.2 for release
It could allow the JIT to spend more JIT time to generate better code, such as inlining more aggressively into the function

CC @noahfalk @vancem @dotnet/jit-contrib

Note that changing this enum is a file format change, not just an API change. ECMA-335 specifies the valid values for this enumeration in section II.22.26 and also says values out of the range of allowed values should be treated as error.

For example, this new value won't roundtrip through ILDASM/ILASM without revving IL syntax.

I see. I have local changes to update ildasm/ilasm. What is the process to propose the change to the spec?

Also is there an IL version (corresponding to spec version) that should be revved and taken into account for providing errors?

I think the language in the spec that says unspecified values are errors should be modified. As a general principle the spec should not prohibit extensions without good reason. If the spec is still accepting updates, we can add that to it, but we should not block on it.

Ideally the situation should be ILASM should have syntax for values it does not know about (e.g hex numbers), and should encode them so that they can be round-tripped. We should fix ILASM/DASM to do this.

I am interested in knowing what the ILASM/DASM tools do (does it strip it? That is not so bad in this case. This will not be the last time we will want another bit to 'hang' on a method. If MethodImplOptions is going to be problematic because tools did fragile things, we may need to use a 'real' custom attribute instead. First we have to find out how bad it is in actual practices (any reader tools blow up?).

I modified a binary to set the new enum value on Main. The baseline ILDASM without changes is stripping the unknown value and not throwing:

  .method private hidebysig static void  Main() cil managed

The modified ILDASM is recognizing the new value:

  .method private hidebysig static void  Main() cil managed aggressiveoptimization

The baseline CoreCLR is not throwing an exception. It looks like there is a flag check when using System.Reflection.Emit.TypeBuilder.DefineCustomAttribute() to throw for invalid unknown flags, which is probably fine.

ILSpy extension in VS is showing the value like this:

[MethodImpl((MethodImplOptions)512)]
private static void Main()

@kouvel

@kouvel, how did you fix ILASM? did you invent a new keyword? That is fine, but in addition it would be nice if we can make ILASM syntax that is like ILSpy's (round tripping works for anything in the future without modification).

I don't have a lot of experience knowing what kind of things break when IL is updated. I think we should still proceed, but if anyone knows of things that might break (e.g. CCI and tools based on it), a heads up would be appropriate.

@jkotas FYI.

Yea I added the new keyword. Maybe something like methodimploptions(512) would be appropriate for generic handling. Are there any tools other than ilasm that parse ildasm's output or IL-source?

I am sure there are a bunch of ad-hoc tools that parse ILASM, but most are probably pretty robust.

The current syntax looks in ILDASM is the following (where 'noinlining' is a MethodImplption).

  .method public hidebysig static void  Main(string[] args) cil managed noinlining

Thus the syntax is simpy a identifier that follows the parameters. While your suggestion of allowing one of these to be methodimploption(number), is reasonable, I worry that it woud 'trip up' simplfied parsers (e.g. regular expressions (.*)). Thus I think it would be slightly better to have code that makes up 'generic' value identifiers. For example methodImplOption_200 (where 200 is the hex value.

Ideally we would do this for other bits that have special sytnax in ILDASM, but I am OK with just doing MethodImplOptions for now (although if people can think of ones that are likely to be augmented, we should do those proactively).

I guess the new method of specification should be in addition to the keywords for back-compat and should be preferred over the keywords if we add new bits in the future without new keywords.

This is not the first time we are allocating a new bit here. I do not recall adding a new keyword for a new bit here ever being a problem.

should be preferred over the keywords

The keywords are there to make the text human readable. I do not think we want to be encouraging numbers that you need to lookup somewhere else.

Any thoughts on what should happen if a method marked for AggressiveOptimization is also an inline candidate?

My inclination would be to not inline such methods unless the root method was also so marked.

I agree with Andy's inclination.

What I like about AgressiveOptimization is that the guidance is straightfoward.

I believe that the guidance for AgressiveOptimization for a method M is the method M is hot (e.g. uses a few % or more of the CPU) in at least one important scenario.

Since a method can be called from many places (some of which are not hot), you should not assume that EVERY place the method is used deserves to be optimized agressvely. Certainly if its caller is marked for AgressiveOptimzation then it should be optimized, but the JIT is allowed to use additional heuristics.

It is another question whether a method is so small and likely to expose more optimization (e.g. a field fetch), that it should be optimized in almost all contexts (the only exception is that you are REALLY trying to save JIT time).

Can someone from the JIT team comment on what 'AgressiveInlining' means? In do you mark the callee or the caller (I am assuming it is the callee). Note AgressiveOptimization does not work like that (if you apply AgressiveOptimziation to M it is not likley M will be inlined, it is likey that M will inline methods it calls.

For what it is worth...

@AndyAyersMS

The spec is not a static thing - even though we are not currently working on updates to the spec, we should plan on having that opportunity in the future.

I like the idea of extending ildasm to flexibly handle as-yet-undefined values, and however we decide to expose them in ildasm we should modify ilasm to interpret them accordingly.

It is another question whether a method is so small and likely to expose more optimization (e.g. a field fetch), that it should be optimized in almost all contexts (the only exception is that you are REALLY trying to save JIT time).

I would think that such a method would be a better candidate for AggressiveInlining than for AggressiveOptimization, but if the latter were specified I think that the approach that @AndyAyersMS suggests would be appropriate - only inline if the caller has AggressiveOptimization.

Can someone from the JIT team comment on what 'AgressiveInlining' means? In do you mark the callee or the caller (I am assuming it is the callee).

You're correct, it is the callee. But I would mildly disagree with this statement:

Note AgressiveOptimization does not work like that (if you apply AgressiveOptimziation to M it is not likley M will be inlined, it is likey that M will inline methods it calls.

In some sense if you apply AggressiveOptimization to M, then it probably makes it more likely that it will be inlined into methods that are also marked AggressiveOptimization, which would allow you to tweak where you really want it inlined into.

Of course, this has the downside of presenting more knobs that can potentially be abused, but I think it's all to the general good.

@AndyAyersMS

would it also be possible to "unseal" the MethodImplAttribute class?

Reason: in BenchmarkDotNet to prevent the benchmark from being inlined we wrap the method call into a delegate (delegate calls are not being inlined yet).

So all the user needs to do is just to apply the "Benchmark" attribute to the method:

[Benchmark]
public void Method () { }

and there is no need to do:

[Benchmark]
[MethodImpl(MethodImplOptions.NoInlining)]
public void Method () { }

If we would like to enforce AggressiveOptimization by default for all of the [Benchmark] methods we would have to ask the users to always write:

[Benchmark]
[MethodImpl(MethodImplOptions.AggressiveOptimization)]
public void Method () { }

If we could unseal the MethodImplAttribute class, then the BenchmarkAttribute could just derive from it and set both NoInlining and AggressiveOptimization by default. The possibility of inlining delegates in the future would not be an issue (@AndyAyersMS @AndreyAkinshin @jorive)

would it also be possible to "unseal" the MethodImplAttribute class?

The .NET Framework design guidelines say "DO seal custom attribute". Dealing with unsealed custom attributes is a lot more expensive. Moreover, MethodImplAttribute is one of the special-pseudo custom attributes. It is recognized at compile time, not at runtime. If we were to unseal it, compilers and other metadata producers would need to be changed to account for this too. Your scenario should be discussed in separate issue, feel free to open one. Unsealing the attribute is not the only way to achieve what you would like to do.

If we would like to enforce AggressiveOptimization by default for all of the [Benchmark] methods

In the future the perf of methods flagged with AggressiveOptimization may diverge from unflagged methods. Whether the benchmark should be flagged I guess could depend on whether real usage is expected to be flagged with the attribute. So my suggestion is to not imply AggressiveOptimization for benchmarks and instead leave the choice to the benchmark author, at least for now.

Looks good as proposed.

Add MethodImplOptions.AggressiveOptimization and use it for tiering (#…

Add MethodImplOptions.AggressiveOptimization and use it for tiering (…

Expose MethodImplOptions.AggressiveOptimization and MethodImplAttribu…

Exposed in dotnet/corefx#32322

Navigation Menu

Repository navigation

[API review] Add MethodImplOptions.AggressiveOptimization #27370

15 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[API review] Add MethodImplOptions.AggressiveOptimization #27370

Description

Activity

MichalStrehovsky commented on Sep 11, 2018

kouvel commented on Sep 11, 2018

kouvel commented on Sep 11, 2018

vancem commented on Sep 11, 2018

kouvel commented on Sep 11, 2018

kouvel commented on Sep 11, 2018

vancem commented on Sep 11, 2018

kouvel commented on Sep 11, 2018

vancem commented on Sep 11, 2018

kouvel commented on Sep 11, 2018

jkotas commented on Sep 11, 2018

AndyAyersMS commented on Sep 11, 2018

vancem commented on Sep 11, 2018

CarolEidt commented on Sep 11, 2018

15 remaining items

adamsitnik commented on Sep 21, 2018

jkotas commented on Sep 21, 2018

kouvel commented on Sep 24, 2018

terrajobst commented on Sep 26, 2018

kouvel commented on Oct 27, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions