Reduce generic exception code bloat in System.Linq

[jamesqo]

Currently System.Linq uses this pattern to handle invalid arguments:

if (source == null)
{
    throw Error.ArgumentNull(nameof(source));
}

Where ArgumentNull gets inlined. This introduces significant jitted code bloat since the Linq methods are generic, so this code (which very rarely gets hit) is generated over and over again for different value types.

We should convert all of this to the form

if (source == null)
{
    Error.ThrowSourceArgumentNull(); // or ThrowArgumentNullSource
}

As of dotnet/coreclr#6103 methods that are known to throw are not inlined, so this should help decrease code size.

cc @benaadams

[stephentoub]

This introduces significant jitted code bloat

Can you share numbers?

[benaadams]

@jamesqo is probably better to go through enum helpers with a function look-up like ThrowHelper in coreclr then they can be reused as the enum is a single numeric stack load. nameof does a string load so is slightly larger; and but combining would mean an exception method for each argument name.

So more

if (source == null)
{
    Error.ThrowArgumentNull(ExceptionArgument.source);
}

Standard type throw new ArgumentNullException(nameof(source)) or an inlined throw becomes

G_M32612_IG08:
       48B95862FACAFD7F0000 mov      rcx, 0x7FFDCAFA6258
       E84E01D05F           call     CORINFO_HELP_NEWSFAST
       488BF0               mov      rsi, rax
       B952020000           mov      ecx, 594
       48BA7854846BFD7F0000 mov      rdx, 0x7FFD6B845478
       E80711AC5F           call     CORINFO_HELP_STRCNS
       488BD0               mov      rdx, rax
       488BCE               mov      rcx, rsi
       E8BCC9125F           call     ArgumentNullException:.ctor(ref):this
       488BCE               mov      rcx, rsi
       E8E43AAC5F           call     CORINFO_HELP_THROW

The enum based throw in callee becomes (in Core 1.1.0+)

**************** Inline Tree
[FAILED: does not return] ThrowHelper:ThrowArgumentNullException(int)

G_M54245_IG08:
       B903000000           mov      ecx, 3
       E89CFBFFFF           call     ThrowHelper:ThrowArgumentNullException(int)
       CC                   int3   

Aside
Have to be careful with too much/conditional logic in the Throwhelper as that can make the inliner decide its not only a throw and change behaviour.

For example if you were changing the exception thrown with some if logic based on arguments you want to add a second step of an Exception generator to keep the Throw helper to a single throw statement without conditionals.

private static void ThrowInvalidArguments(string buffer, int offset, int length)
{
    throw GetInvalidArgumentException(buffer, offset, length);
} 

@stephentoub this is just an illustration of the difference in code gen; I don't know what the particular metrics change would be in this case. Though if the execption generation methods are inlined they would be inlined once for each valuetype the methods are used for (and once for all object types)

[VSadov]

Replacing literal "throw" with not inlinable call seems like a good idea.

Whether to use enum-based dispatch - seems like a good idea too, but the win is not as clear. Can we have more info on that?

[stephentoub]

This was fixed by dotnet/corefx#35213.