Remove Generic<T> Exception code repetition #6634
Conversation
| throw new ArgumentNullException("elementType"); | ||
| } | ||
|
|
||
| private static void ThrowArgumenCountOutOfRange() |
|
Assume fixing a typo didn't break the tests O_o |
| throw new ArgumentException(Environment.GetResourceString("Arg_MustBeType"), "elementType"); | ||
| } | ||
|
|
||
| private static void ThrowArgumentOutOfRangeNeedNonNegNum(string parmName) |
|
Unfortunately this change breaks the way that static void CommonlyUsedGenericInstantiations()
{
System.Array.Sort<double>(null);
System.Array.Sort<int>(null);
System.Array.Sort<IntPtr>(null);
new ArraySegment<byte>(new byte[1], 0, 0);
...resulting asm with this change: _M1895_IG01:
sub rsp, 120
xor rax, rax
mov qword ptr [rsp+68H], rax
mov qword ptr [rsp+70H], rax
G_M1895_IG02:
call Array:ThrowArrayNullException()
int3The jit now can inline the first |
|
Not sure I understand, wouldn't the previous behaviour have the same effect? public static void Sort<T>(T[] array)
{
if (array == null)
throw new ArgumentNullException("array");
Contract.EndContractBlock();
Sort<T>(array, 0, array.Length, null);
}Should I back out the |
|
Also is it worth adding some Array.Empty to that list? e.g. System.Array.Empty<byte>();
System.Array.Empty<char>();
System.Array.Empty<int>();
System.Array.Empty<uint>();
System.Array.Empty<long>();
System.Array.Empty<ulong>();
System.Array.Empty<object>(); |
|
Best of both worlds? Array.Sort<double>(Array.Empty<double>());
Array.Sort<int>(Array.Empty<int>());
Array.Sort<IntPtr>(Array.Empty<IntPtr>()); |
benaadams
force-pushed
the
array-exceptions
branch
from
37caf55 to
02ff7c9
Compare
|
Adding the |
benaadams
force-pushed
the
array-exceptions
branch
from
02ff7c9 to
4570a40
Compare
|
Removed some more repeated exceptions to functions specically in Search path, down to 10,889,216 bytes So saving 24064 bytes on disk or 24kB |
| @@ -2636,6 +2636,47 @@ public Object Clone() | |||
| [System.Security.SecuritySafeCritical] // auto-generated | |||
| [MethodImplAttribute(MethodImplOptions.InternalCall)] | |||
| public extern void Initialize(); | |||
|
|
|||
|
|
|||
| private static void ThrowArrayNullException() | |||
There was a problem hiding this comment.
You should look at the existing ThrowHelper class and use that whenever possible. It's not very useful if we keep adding such methods all other the place.
There was a problem hiding this comment.
Changed all the calls to ThrowHelper rather than the statics
|
Should be able to push a lot of this to ThrowHelper |
| @@ -45,7 +45,7 @@ public abstract class Array : ICloneable, IList, IStructuralComparable, IStructu | |||
| [ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)] | |||
| public static void Resize<T>(ref T[] array, int newSize) { | |||
| if (newSize < 0) | |||
| throw new ArgumentOutOfRangeException("newSize", Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum")); | |||
| ThrowArgumentOutOfRangeNeedNonNegNum(nameof(newSize)); | |||
There was a problem hiding this comment.
Use of constant strings results in an additional call, that's why ThrowHelper passes the argument as an enum instead.
There was a problem hiding this comment.
The diffs are a bit easier to compare with ThrowHelper also 😄
|
Down to 10,887,168 bytes on disk with ThrowHelper saving 26112 bytes or 25kB Also now none of the generic methods throw directly. |
benaadams
force-pushed
the
array-exceptions
branch
2 times, most recently
from
7163100 to
385b3b6
Compare
|
Down to 10,885,632 bytes with ArraySegment; 27,648 byte saving |
|
@dotnet-bot retest Windows_NT x64 Release Priority 1 Build and Test |
|
Circling back a bit to my note on the change to codegen for |
|
Would that suggest it would be sensible to go straight for |
|
It also drops it a few more bytes to 10,884,608 bytes (1024 more?) |
|
I believe the intent of The top-level Sort calls are getting inlined. @jkotas can correct me here but I believe that means these Sort methods will not get instantiated as expected, since the instantiation closure process is driven by "fixup" references left in the final code stream. So my preference would be mark I am not sure if the empty array variants are used often enough to warrant forced instantiation, but likely it is fairly harmless to add them. The change to consistently use Sorting all this out is going to take some work, so for now I'd just fix the issue with You can see the range of behaviors on display in G_M62283_IG01:
push rdi
push rsi
push rbp
push rbx
sub rsp, 40
xor rax, rax
mov qword ptr [rsp+20H], rax
mov rsi, rdx
G_M62283_IG02:
test rcx, rcx
je G_M62283_IG16
G_M62283_IG03:
test rsi, rsi
je G_M62283_IG17
G_M62283_IG04:
mov edi, dword ptr [rsi+8]
test edi, edi
je G_M62283_IG18
G_M62283_IG05:
mov rax, qword ptr [rcx]
mov rax, qword ptr [rax+152]
call gword ptr [rax+40]Type:get_UnderlyingSystemType():ref:this
mov rdx, rax
lea rcx, [(reloc)]
call CORINFO_HELP_ISINSTANCEOFCLASS
mov rbx, rax
test rbx, rbx
jne SHORT G_M62283_IG06
mov ecx, 49
mov edx, 27
call ThrowHelper:ThrowArgumentException(int,int) // not deduced no-return
G_M62283_IG06:
xor ebp, ebp
test edi, edi
jle SHORT G_M62283_IG09
G_M62283_IG07:
movsxd rcx, ebp
cmp dword ptr [rsi+4*rcx+16], 0
jl G_M62283_IG19
G_M62283_IG08:
inc ebp
cmp edi, ebp
jg SHORT G_M62283_IG07
G_M62283_IG09:
test edi, edi
jne SHORT G_M62283_IG11
G_M62283_IG10:
xor rcx, rcx
mov bword ptr [rsp+20H], rcx
jmp SHORT G_M62283_IG12
G_M62283_IG11:
cmp edi, 0
jbe G_M62283_IG20
add rsi, 16
mov bword ptr [rsp+20H], rsi
G_M62283_IG12:
mov rcx, rbx
mov rax, qword ptr [rbx]
mov rax, qword ptr [rax+88]
call gword ptr [rax+24]Type:get_TypeHandle():struct:this
test rax, rax
jne SHORT G_M62283_IG13
xor rcx, rcx
jmp SHORT G_M62283_IG14
G_M62283_IG13:
mov rcx, qword ptr [rax+24]
G_M62283_IG14:
mov edx, edi
mov r8, bword ptr [rsp+20H]
xor r9, r9
call Array:InternalCreate(long,int,long,long):ref
nop
G_M62283_IG15:
add rsp, 40
pop rbx
pop rbp
pop rsi
pop rdi
ret
************** Beginning of cold code **************
G_M62283_IG16:
mov ecx, 27
call ThrowHelper:ThrowArgumentNullException(int) // moved, deduced no return
G_M62283_IG17:
mov ecx, 34
call ThrowHelper:ThrowArgumentNullException(int) // moved, deduced no return
G_M62283_IG18:
lea rcx, [(reloc)]
call CORINFO_HELP_NEWSFAST
mov rbp, rax
mov ecx, 0x10072
call CORINFO_HELP_STRCNS_CURRENT_MODULE
mov rcx, rax
call Environment:GetResourceStringLocal(ref):ref
mov rdx, rax
mov rcx, rbp
call ArgumentException:.ctor(ref):this // moved, inlined
mov rcx, rbp
call CORINFO_HELP_THROW
G_M62283_IG19:
lea rcx, [(reloc)]
call CORINFO_HELP_NEWSFAST
mov rsi, rax
mov ecx, 0x1009C
call CORINFO_HELP_STRCNS_CURRENT_MODULE
mov rdi, rax
mov dword ptr [rsi+8], ebp
mov ecx, 847
call CORINFO_HELP_STRCNS_CURRENT_MODULE
mov r8, rax
mov rcx, rdi
mov rdx, rsi
call String:Concat(ref,ref,ref):ref
mov rsi, rax
lea rcx, [(reloc)]
call CORINFO_HELP_NEWSFAST
mov rdi, rax
mov ecx, 0x4D2F
call CORINFO_HELP_STRCNS_CURRENT_MODULE
mov rcx, rax
call Environment:GetResourceStringLocal(ref):ref
mov rbx, rax
mov rcx, rdi
call Exception:Init():this
lea rcx, bword ptr [rdi+32]
mov rdx, rbx
call CORINFO_HELP_ASSIGN_REF
mov dword ptr [rdi+132], 0xD1FFAB1E
lea rcx, bword ptr [rdi+144]
mov rdx, rsi
call CORINFO_HELP_ASSIGN_REF
mov dword ptr [rdi+132], 0xD1FFAB1E
mov dword ptr [rdi+132], 0xD1FFAB1E
mov rcx, rdi
call CORINFO_HELP_THROW // moved, inlined
int3
G_M62283_IG20:
call CORINFO_HELP_RNGCHKFAIL
int3 |
|
Marked as such. Up a bit to 10,885,120 bytes so 28,160 bytes saving |
|
@AndyAyersMS thank you for working though this with me. Are the various outputs from jitutils? (dasm, and inlining choices) |
|
There are a couple throws I haven't moved out to the ThrowHelper in non-generic methods (for example in CreateInstance). Shall I do those also? There are a couple oddities: ThrowHelper.ThrowArgumentOutOfRangeException(
length1 < 0 ? ExceptionArgument.length1 : ExceptionArgument.length2,
ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);Which I could split two 2 ifs? It would only leave the 3 which have variable parameter names e.g. throw new ArgumentOutOfRangeException("lengths[" + i + ']',
Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum"));Which I could move out to a static static void ThrowLengthOutOfRange(int parameterNumber)
{
throw new ArgumentOutOfRangeException("lengths[" + parameterNumber + ']',
Environment.GetResourceString("ArgumentOutOfRange_NeedNonNegNum"));
}Then see how it looks? |
benaadams
force-pushed
the
array-exceptions
branch
from
5af409b to
ce291b4
Compare
|
Rebased |
|
@dotnet-bot test Windows_NT x86 ryujit Checked Build and Test |
|
Closing this as I think most of the big individual ones are done
The remaining ones in the concurrent collections are in the interface path (hopefully not common use) |
With the introduction of #6103 "Do not inline methods that never return"
These changes reduce the code gen size of the jitted methods (regular and generic * N) for Array and ArraySegment. (And a bunch of other generic types)
Also results in a 55kB reduction int the size of System.Private.CoreLib native (Windows_NT.x64.Release).
55,808 byte reduction
One of the big wins, other than the included throw code is smaller (21 bytes of il to 7 bytes), is it prevents all the inlining of
Environment:GetResourceString(ref):refe.g.Pre
Post
Which will be repeated for each
new Generic<struct>orMethod<struct>(...)flavour.