RyuJIT should not inline throw only methods

[mikedn]

The ThrowArgsNull method in the following example gets inlined even though there's no reason to do so, throwing exceptions isn't fast to begin with and such codepaths aren't supposed to be common anyway. The inlined code size is significantly larger than the call site (in this example 57 bytes vs 5 bytes).

using System;
class Program {
    static void Main(string[] args) {
        if (args == null)
            ThrowArgsNull();
    }
    static void ThrowArgsNull() {
        throw new ArgumentNullException("args");
    }
}

More generally, there's probably no reason to inline any method that doesn't contain at least one BBJ_RETURN block.

[ayende]

Why would you want such a method anyway?

Usually throw only methods are done because you have complex code to create
the exception (creating good error message).

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Wed, Jul 22, 2015 at 8:28 AM, mikedn [email protected] wrote:

The ThrowArgsNull method in the following example gets inlined even
though there's no reason to do so, throwing exceptions isn't fast to begin
with and such codepaths aren't supposed to be common anyway. The inlined
code size is significantly larger than the call site (in this example 57
bytes vs 5 bytes).

using System;class Program {
static void Main(string[] args) {
if (args == null)
ThrowArgsNull();
}
static void ThrowArgsNull() {
throw new ArgumentNullException("args");
}
}

More generally, there's probably no reason to inline any method that
doesn't contain at least one BBJ_RETURN block.

—
Reply to this email directly or view it on GitHub
https://github.com/dotnet/coreclr/issues/1270.

[mikedn]

Why would you want such a method anyway?

Exactly because I didn't expect such a method to be inlined and didn't want to bloat the caller with useless code. I discovered this in the following code:

unsafe struct ByteStream {
    ...
    private byte* Read(uint count) {
        uint p = rva;
        uint n = p + count;

        if (n > rvaEnd)
            ThrowInvalidOperationException();

        rva = n;
        return basePtr + p;
    }
    public uint ReadByte() {
        return *Read(1);
    }
    private void ThrowInvalidOperationException() {
        throw new InvalidOperationException();
    }

While looking at the generated code I discovered that ReadByte was inlined into its caller (as expected) but dragged in all the throw stuff as well. It did so in multiple places and the caller is pretty large even without the throw stuff being replicated all other the place.

But more generally this is useful when doing argument validation and the example I posted originally is meant to show that.

Of course, there's a trivial workaround, add the [MethodImpl(MethodImplOptions.NoInlining)] attribute to the throw method. But not everyone knows that. Apparently this includes BCL authors who already use this technique: http://referencesource.microsoft.com/#mscorlib/system/throwhelper.cs 😄

[mikedn]

Speaking of BCL, the size of crossgened mscorlib is 18,435,584 bytes. If the fix is applied the size is 18,320,384 bytes, that's some 100 kbytes less.

[BruceForstall]

We've talked about doing this; it would be a good improvement.

[mikedn]

@BruceForstall Does relying on the lack of return basic blocks sound like a good approach for fixing this? It seems to work fine for cases like the one I posted. It won't work if there are return blocks that are later eliminated as dead code but I suspect that's difficult to fix given the way inlining decisions are taken.

[BruceForstall]

That sounds like a good approach. It should handle the most likely case of simple "throw" function wrappers.

[omariom]

I used to wrap throws in throw only methods because I thought throw prevents method inlining and bloats method body size.

[mikedn]

@BruceForstall Thanks, I managed to convince crossgen to dump the disassembly for mscorlib and it looks pretty good. Initially I was a bit confused by some associations between jumps and BBJ_RETURN until I realized that they're about the IL jmp instruction which is very different from the x86 jmp instruction.

There is one small negative side effect of not inlining such methods - the compiler doesn't know that they never return and in some cases it moves registers around to avoid having them killed by the call. I suppose it would be nice to avoid this if possible.

[mikedn]

Maybe @AndyAyersMS would be interested in looking at this. Not necessarily at the inlining decision itself which shouldn't be a problem to fix but at propagating the "no return" information from the inlinee compiler to the inliner. No return methods should be InlineDecision::NEVER but the "no return" reason should also be recorded so it can be used by the inliner for future optimizations.

[omariom]

Would be nice to have an attribute with which we could mark methods. It would require verification that a marked method either throws or calls FailFast.
Then JIT could optimize code around the call site, as if it were a throw statement, and probably move it to the end of the caller.

@mikedn Is that what you mean with 'no return methods'?

[omariom]

Recent PR on the subject.

[ayende]

Isn't that what [MethodImpl(MethodOptions.NoInlining)] for?
What is the purpose of this attribute you are suggesting?

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Sat, Feb 6, 2016 at 9:54 PM, OmariO [email protected] wrote:

Would be nice to have an attribute with which we could mark methods. It
would require verification that a marked method either throws or calls
FailFast.
Then JIT could optimize code around the call site, as if it were a throw
statement, and probably move it to the end of the caller.

@mikedn https://github.com/mikedn Is that what you mean with 'no return
methods'?

—
Reply to this email directly or view it on GitHub
https://github.com/dotnet/coreclr/issues/1270#issuecomment-180846852.

[AndyAyersMS]

Yes, I intend to look at inlining in exceptional contexts. I've seen a number of examples where I think the jit inlines too much along paths leading to exceptions.

Declaring or deducing that a method is no return is an interesting idea, though I can't say if we have all the runtime guarantees and compiler machinery necessary to make safe and effective use of this information.

@ayende Noinlining is kind of a blunt hammer. The jit has no idea why you put this attribute on the method. You could mean "inlining this method is a bad idea for reason X" or you could mean "inlining this method will break scenario Y". So in principle, finer-grained information about when or why this method should not be inlined might be useful. But it's easy to go overboard here too. I'd be happy for now just if we can just make the inliner a bit smarter on its own.

[mikedn]

@omariom

Then JIT could optimize code around the call site, as if it were a throw statement, and probably move it to the end of the caller.

Moving to the end is not very useful. The real advantage of having "no return" information is avoiding unnecessary register moves/saves. See the following example (adapted from List<T>'s indexer):

class Program {
    class List<T> {
        private T[] array = new T[1];
        private int count = 1;
        public T this[int index] {
            get {
                if (index >= count) ThrowArgumentOutOfRangeException();
                return array[index];
            }
        }
    }
    [MethodImpl(MethodImplOptions.NoInlining)]
    static void ThrowArgumentOutOfRangeException() {
        throw new ArgumentOutOfRangeException("index");
    }
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test(List<int> list, int index) {
        return list[index];
    }
    static int Main() {
        return Test(new List<int>(), 42);
    }
}

Code generated for Test:

G_M31016_IG01:
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       488BF9               mov      rdi, rcx
       8BF2                 mov      esi, edx

G_M31016_IG02:
       3B7710               cmp      esi, dword ptr [rdi+16]
       7C05                 jl       SHORT G_M31016_IG03
       E873FBFFFF           call     Program:ThrowArgumentOutOfRangeException()

G_M31016_IG03:
       488B4708             mov      rax, gword ptr [rdi+8]
       3B7008               cmp      esi, dword ptr [rax+8]
       730E                 jae      SHORT G_M31016_IG05
       4863D6               movsxd   rdx, esi
       8B449010             mov      eax, dword ptr [rax+4*rdx+16]

G_M31016_IG04:
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret

G_M31016_IG05:
       E88F70D25E           call     CORINFO_HELP_RNGCHKFAIL
       CC                   int3

Because the JIT compiler thinks that ThrowArgumentOutOfRangeException returns it has to save ecx and edx so they don't get killed by the call. And it does that by moving ecx and edx to the call preserved registers rdi and rsi. In turn, that requires saving/restoring rdi and rsi in the function prolog/epilog.

As you have seen in a corefxlab PR keeping the throw out of the helper method avoids the register issue:

G_M31938_IG01:
       4883EC28             sub      rsp, 40

G_M31938_IG02:
       3B5110               cmp      edx, dword ptr [rcx+16]
       7D15                 jge      SHORT G_M31938_IG05

G_M31938_IG03:
       488B4108             mov      rax, gword ptr [rcx+8]
       3B5008               cmp      edx, dword ptr [rax+8]
       731A                 jae      SHORT G_M31938_IG06
       4863D2               movsxd   rdx, edx
       8B449010             mov      eax, dword ptr [rax+4*rdx+16]

G_M31938_IG04:
       4883C428             add      rsp, 40
       C3                   ret

G_M31938_IG05:
       E835FBFFFF           call     Program:ThrowArgumentOutOfRangeException():ref
       488BC8               mov      rcx, rax
       E8E507D05E           call     CORINFO_HELP_THROW
       CC                   int3

G_M31938_IG06:
       E85F70D05E           call     CORINFO_HELP_RNGCHKFAIL
       CC                   int3

I had to dump the disassembly for get_Item because if the throw is kept out of the helper then get_Item is no longer inlined in Test. But it can be seen that there are no more register saves, the JIT compiler knows that CORINFO_HELP_THROW does not return (well, it probably knows that the throw instruction doesn't return, I don't think there's any way to encode a "no return" call in IR right now).

But keeping the throw out of the helper means that the throw code is now slightly larger. Without JIT support it's not possible to get ideal code for argument validation. And argument validation is quite common in .NET...

[omariom]

@mikedn
I think JIT even today could treat the methods that unconditionally throw as moral throw equivalents.
If the call site is not impacted by servicing updates, of course.

btw, is servicing updates a concern in CoreCLR?

[ayende]

@omario what does it mean, service updates? Service packs?

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Sun, Feb 7, 2016 at 12:30 PM, OmariO [email protected] wrote:

@mikedn https://github.com/mikedn
I think JIT even today could treat the methods that unconditionally throw
as moral throw equivalents.
If the call site is not impacted by servicing updates, of course.

btw, is servicing updates a concern in CoreCLR?

—
Reply to this email directly or view it on GitHub
https://github.com/dotnet/coreclr/issues/1270#issuecomment-180992980.

[omariom]

@ayende
I meant to say servicing update )

If understand it correctly because of them JIT (which does NGEN as well) is restricted in some optimizations it can do. For example, when a sealed method becomes unsealed or when unconditionally throwing method start throwing conditionally.

[zpodlovics]

@mikedn
@dsyme

Please do NOT remove throw only methods inlining because F# already use it in NoDynamicInvocation attribute implementations.

According to the language spec (3.1) NoDynamicInvocation means:

When applied to an inline function or member definition, replaces the generated code with a stub that throws an exception at runtime. This attribute is used to replace the default generated implementation of unverifiable inline members with a verifiable stub. This attribute should be used only in F# assemblies.

https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/prim-types.fs#L689

[dsyme]

@zpodlovics That's not quite accurate - the F# compiler does it's own inlining on the actual implementation of these pseudo-methods which are known only to the compiler - so it's still correct for RyuJIT to avoid inlining them

[briansull]

When we consider such a method for inlining we should:
1: Decline to inline it
2: Mark the block that contains the call as RarelyRun

[mikedn]

@briansull I experimented with this when reporting the issue and rejecting inlining seemed easy enough. I suppose marking the block as RarelyRun should be trivial but that's not enough to get best CQ. The BB kind should also be changed to something that works like BBJ_THROW, otherwise you end up with unnecessary registers spills/moves.

[choikwa]

@mikedn, speaking of Register save/restore, does CoreCLR have support for leaf methods? If it's a todo, this could fall under it (noReturn or alwaysThrows attribute means reg save/restore could be elided and ThrowArgumentOutOfRangeException() block becomes an exit).

[mikedn]

@choikwa I'm not sure what you mean by "support for leaf methods" and how such support relates to this issue. Usually a "leaf method" is a method that does not call other methods, not a method that does not return.

[choikwa]

Nevermind, what I was thinking of would apply to unmanaged code. Certain leaf methods can be scanned by caller to skip save/restoring if it can determine that callee won't use them.

[mikedn]

Fixed by dotnet/coreclr#6103