Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

JIT: devirtualization support for EqualityComparer<T>.Default #14125

Merged
merged 1 commit into from
Sep 27, 2017

Conversation

AndyAyersMS
Copy link
Member

Mark EqualityComparer<T>.Default's getter as [Intrinsic] so
the jit knows there is something special about it. Extend the jit's
named intrinsic recognizer to recognize this method.

Add a new jit interface method to determine the exact type returned
by EqualityComparer<T>.Default, given T. Compute the return type by
mirroring the logic used in the actual implementation.

Invoke this interface method when trying to devirtualize calls where
the 'this' object in the call comes from EqualityComparer<T>.Default.
Handle both the early and late devirtualization possibilties.

The devirtualized method can then be inlined if devirtualization
happens early; inlining uses the normal jit heuristics here.

Closes #6688.

@AndyAyersMS AndyAyersMS added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 21, 2017
@AndyAyersMS
Copy link
Member Author

Still some work to do to hook this up to SPMI, bump the jit guid, update desktop, etc, but want to settle on the shape and content of the jit interface changes first and get a little testing underway.

@davidwrighton @jkotas PTAL
cc @dotnet/jit-contrib

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JIT-EE interface change looks good to me.

{
case ELEMENT_TYPE_I1:
{
targetClass = MscorlibBinder::GetClass(CLASS__SHORT_ENUM_EQUALITYCOMPARER);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy&paste mistake - SHORT_ENUM_EQUALITYCOMPARER should be for I2.

@AndyAyersMS
Copy link
Member Author

Sample codegen inspired by this test method:

class T
{
    enum E { ... }

    public static bool Compare(ref ValueTuple<byte, E, int> a, ref ValueTuple<byte, E, int> b)
    {
        return a.Equals(b);
    }
}

Note that the calls to Equals all devirtualize, but only the byte comparer gets inlined. We might want to mark some of these comparer methods for aggressive inlining, since the jit is fooled in the Enum case by the apparent cost of the cast helpers, and in the Generic case by the apparent cost of the null checks, both of which should optimize down nicely.

Also wonder if there's anything clever we can do about the class init calls here...

Devirtualized virtual call to System.Collections.Generic.EqualityComparer`1[Byte]:Equals; now direct call to System.Collections.Generic.ByteEqualityComparer:Equals [exact]
Devirtualized virtual call to System.Collections.Generic.EqualityComparer`1[E]:Equals; now direct call to System.Collections.Generic.EnumEqualityComparer`1[E][T+E]:Equals [exact]
Devirtualized virtual call to System.Collections.Generic.EqualityComparer`1[Int32]:Equals; now direct call to System.Collections.Generic.GenericEqualityComparer`1[Int32][System.Int32]:Equals [exact]

Inlines into 06001786 System.ValueTuple`3[Byte,E,Int32][System.Byte,T+E,System.Int32]:Equals(struct):bool:this
  [1 IL=0000 TR=000001 060039A1] [below ALWAYS_INLINE size] System.Collections.Generic.EqualityComparer`1[Byte][System.Byte]:get_Default():ref
  [2 IL=0017 TR=000009 060039C2] [below ALWAYS_INLINE size] System.Collections.Generic.ByteEqualityComparer:Equals(ubyte,ubyte):bool:this
  [3 IL=0024 TR=000023 060039A1] [below ALWAYS_INLINE size] System.Collections.Generic.EqualityComparer`1[E][T+E]:get_Default():ref
  [0 IL=0041 TR=000031 060039C9] [FAILED: unprofitable inline] System.Collections.Generic.EnumEqualityComparer`1[E][T+E]:Equals(int,int):bool:this
  [4 IL=0048 TR=000041 060039A1] [below ALWAYS_INLINE size] System.Collections.Generic.EqualityComparer`1[Int32][System.Int32]:get_Default():ref
  [0 IL=0065 TR=000049 060039AA] [FAILED: noinline per IL/cached result] System.Collections.Generic.GenericEqualityComparer`1[Int32][System.Int32]:Equals(int,int):bool:this

; Assembly listing for method System.ValueTuple`3[Byte,E,Int32][System.Byte,T+E,System.Int32]:Equals(struct):bool:this
; Emitting BLENDED_CODE for X64 CPU with AVX
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 this         [V00,T01] (  5,  4   )   byref  ->  rdi         this
;  V01 arg1         [V01,T00] (  5,  8   )   byref  ->  rsi
;  V02 tmp0         [V02,T02] (  2,  4   )   ubyte  ->  rcx
;  V03 tmp1         [V03,T03] (  2,  4   )   ubyte  ->  rdx
;* V04 tmp2         [V04    ] (  0,  0   )     int  ->  zero-ref    V07.Item2(offs=0x00) P-INDEP
;* V05 tmp3         [V05    ] (  0,  0   )     int  ->  zero-ref    V07.Item3(offs=0x04) P-INDEP
;* V06 tmp4         [V06    ] (  0,  0   )   ubyte  ->  zero-ref    V07.Item1(offs=0x08) P-INDEP
;* V07 tmp5         [V07    ] (  0,  0   )  struct (16) zero-ref
;  V08 tmp6         [V08,T04] (  2,  2   )     ref  ->  rcx
;  V09 tmp7         [V09,T05] (  2,  2   )     ref  ->  rcx
;  V10 OutArgs      [V10    ] (  1,  1   )  lclBlk (32) [rsp+0x00]
;
; Lcl frame size = 40

G_M31421_IG01:
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       488BF9               mov      rdi, rcx
       488BF2               mov      rsi, rdx

G_M31421_IG02:
       48B928305B41FF7F0000 mov      rcx, 0x7FFF415B3028
       BA01000000           mov      edx, 1
       E80081375F           call     CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS
       0FB64F08             movzx    rcx, byte  ptr [rdi+8]
       0FB65608             movzx    rdx, byte  ptr [rsi+8]
       3BCA                 cmp      ecx, edx
       7567                 jne      SHORT G_M31421_IG04
       48B9F8555B41FF7F0000 mov      rcx, 0x7FFF415B55F8
       33D2                 xor      edx, edx
       E8E380375F           call     CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS
       48B928288C6807020000 mov      rcx, 0x207688C2828
       488B09               mov      rcx, gword ptr [rcx]
       8B17                 mov      edx, dword ptr [rdi]
       448B06               mov      r8d, dword ptr [rsi]
       E8A4FCFFFF           call     System.Collections.Generic.EnumEqualityComparer`1[E][T+E]:Equals(int,int):bool:this
       85C0                 test     eax, eax
       743B                 je       SHORT G_M31421_IG04
       48B928305B41FF7F0000 mov      rcx, 0x7FFF415B3028
       BA4F000000           mov      edx, 79
       E8B480375F           call     CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS
       48B930288C6807020000 mov      rcx, 0x207688C2830
       488B09               mov      rcx, gword ptr [rcx]
       8B5704               mov      edx, dword ptr [rdi+4]
       448B4604             mov      r8d, dword ptr [rsi+4]
       48B8406340A0FF7F0000 mov      rax, 0x7FFFA0406340

G_M31421_IG03:
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       48FFE0               rex.jmp  rax

G_M31421_IG04:
       33C0                 xor      eax, eax

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

; Total bytes of code 156, prolog size 6 for method System.ValueTuple`3[Byte,E,Int32][System.Byte,T+E,System.Int32]:Equals(struct):bool:this

@tannergooding
Copy link
Member

@AndyAyersMS, just as an FYI. I have a PR which is adding new named intrinsics as well: #14119

One of us will probably end up needing to change our enum values, depending on who gets merged first

@AndyAyersMS
Copy link
Member Author

Yep, was just looking at your change.

Likely your change will merge first. I need to make anticipatory changes over in desktop before mine goes into CoreCLR (the jit source is mirrored but the jit interface sources are not) and also work on all the various SPMI implementations.

@benaadams
Copy link
Member

Does it treat IEquatable structs and classes differently?

i.e. class would need to be v0?.Equals(v1) ?? v1 == null whereas struct is just v0.Equals(v1)

@benaadams
Copy link
Member

benaadams commented Sep 22, 2017

Added change for the intrinsic to the Dict change have been working on d0c3b35

@AndyAyersMS
Copy link
Member Author

Note that the scope of devirtualization here is very narrow -- the comparer call must literally be directly made off of the value produced by get_Default. Calls via a field or local that holds a value produced by get_Default will not get optimized.

The dynamic class init call / static base fetch included in get_Default in is around 30 instructions when the class is already initialized. This is likely larger than the instruction savings from devirtualization and inlining. So while existing uses of get_Default will see some benefit, it is not generally beneficial to "forward prop" these calls closer to their uses if the get_Default is moving into more frequently executed code.

For instance, it not going to be easy to fix code like Dictionary<TKey, Tvalue> to see improvements here.

I did some ad-hoc alteration to the jit to suppress the class init check in this case since at least a cursory scan of the comparer methods returned by get_Default showed they don't actually use the object instance, so we don't need the static base address for anything. This plus some dual-pathing in Dictionary<TKey, TValue> to split out the default cases showed a modest 5% speedup in TryInsert for integer keys, and I think that is about as much as one can expect, at least with simple key types.

@jkotas
Copy link
Member

jkotas commented Sep 23, 2017

I did some ad-hoc alteration to the jit to suppress the class init check in this case

Are you be happy with doing this, or do you think it would be better to treat the whole EqualityComparer<T>.Default.Equals as intrinsic and replace it with a call to a static method?

@AndyAyersMS
Copy link
Member Author

It is fairly surgical, so yeah, I think perhaps we could live with it. The jit would simply delete the code produced by EqualityComparer<T>.Default when devirtualization happens, swapping in a null 'this' instead. I'd need to tweak the inliner a bit too, but it's doable.

Doing that that plus adding some aggressive inlining attributes on the comparer methods (since they are much simpler than they appear from an IL scan) and we'd be in a pretty good place wherever EqualityComparer<T>.Default is directly used.

We'd still need to duplicate code in Dictionary and other classes with similar custom/default comparer logic somehow before they would see any benefit.

Since the jit knows the exact type of the default comparer, if devirtualization fails at some call site where the comparer type is IEqualityComparer<T> and T is known, the jit could do the duplication behind the scenes, injecting a runtime type test, which, if it really just boils down to a method table compare, might be cheap enough that it would also pay off. In other words, transform the comparer calls to the rough equivalent of

if (typeof(comparer) == typeof(EqualityComparer<T>.Default)) // hopefully just *this == constant
    comparer.SpecificComparer<T>.Equals(x,y);  // devirtualized, possibly inlined
else
    comparer.Equals(x,y); // fallback interface call for custom comparer cases

If we do this, we'd possibly get some improvements without touching the collection source code.

@AndyAyersMS
Copy link
Member Author

Last commit is probably too aggressive with this bashing. Need to look more closely at the methods that operate directly on the comparer like Equals(Object obj). Since these generally just look at the type and this is known we might still be ok. If not, we'll have to be more selective about bashing this.

An alternative is to clear out the side effects of the this tree during special devirt and then let normal dead code optimization remove the all the code producing this if it is unused. It wouldn't get the case where the call is devirtualized but not inlined but that might be ok. Or, leave the tree as is and do special cleanup in the inliner if the this is unused, somewhat like we do for box.

@AndyAyersMS AndyAyersMS force-pushed the EqualityComparerDefault branch from 3c792c1 to d547b8a Compare September 24, 2017 16:35
@AndyAyersMS
Copy link
Member Author

Rebased on top of changes from #14119. Filled in SPMI parts and changed the jit GUID.

Changes are getting closer but not quite ready. Still need to look at a few things:

  • Back off on devirt for cases where T is a ref type (or at least a non-final ref type), since the devirt + inlining doesn't accomplish that much.
  • Generalize the inliner's pattern for helper call removal; static field access looks different when prejitting than it does when jitting.

@AndyAyersMS
Copy link
Member Author

Ok, code is probably in its final form. ValueTuple example from above now gives:

       0FB64108             movzx    rax, byte  ptr [rcx+8]
       440FB64208           movzx    r8, byte  ptr [rdx+8]
       413BC0               cmp      eax, r8d
       7519                 jne      SHORT G_M4626_IG04
       8B01                 mov      eax, dword ptr [rcx]
       448B02               mov      r8d, dword ptr [rdx]
       413BC0               cmp      eax, r8d
       750F                 jne      SHORT G_M4626_IG04
       8B4104               mov      eax, dword ptr [rcx+4]
       8B5204               mov      edx, dword ptr [rdx+4]
       3BC2                 cmp      eax, edx
       0F94C0               sete     al
       0FB6C0               movzx    rax, al

G_M4626_IG03:
       C3                   ret

G_M4626_IG04:
       33C0                 xor      eax, eax

G_M4626_IG05:
       C3                   ret

@AndyAyersMS AndyAyersMS removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 25, 2017
@AndyAyersMS
Copy link
Member Author

@dotnet/jit-contrib anyone want to take a look at the jit changes?

@AndyAyersMS
Copy link
Member Author

@JosephTremoulet maybe I can bug you to review this?

Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only reviewed the JIT files. Other than some minor suggestions LGTM.

// If we're jitting the special EqualityComparer<T>.Default
// intrinsic, we can remove the shared helper call if the
// associated field lookup is unused.
if ((info.compFlags & CORINFO_FLG_JIT_INTRINSIC) != 0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion: you might want to say that we are marking it so that later, if it is unused, we can remove it. Otherwise it almost reads as if the code here should be doing the removal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -23074,6 +23087,8 @@ GenTreePtr Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
if (argInfo.argHasSideEff)
{
noway_assert(argInfo.argIsUsed == false);
newStmt = nullptr;
bool noAppend = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I think it's always better to avoid a double negative, so I would name this append and initialize it to true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// This helper call is marked as a "Special DCE" helper during
// importation, over in fgGetStaticsCCtorHelper.
//
// (2) NYI. If after, tunneling through GT_RET_VALs, we find that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: omit the first ','

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that if the comma after GT_RET_VALs is there, then the first comma needs to be there too, just that it goes after "If" rather than "after" (since "after tunneling through GT_RET_VALs" is the dependent clause, modifying "find", that the commas set apart). But apparently I think that "if the comma after GT_RET_VALs is there" is somehow an exception to that rule...

@@ -1026,6 +1026,8 @@ struct GenTree
// of the static field; in both of those cases, the constant
// is what gets flagged.

#define GTF_ICON_NULLTHIS 0x01000000 // GT_CNS_INT -- null constant is the this in a call, ok to inline call

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Perhaps it's just me, but I found it difficult to parse and understand this comment. Would it be accurate to say:

// GT_CNS_INT -- This contant is a null that is the 'this' argument to a call; it is ok to inline this call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this isn't needed anymore, I'll just revert it.

@@ -4997,8 +5001,7 @@ struct GenTreeStoreInd : public GenTreeIndir

struct GenTreeRetExpr : public GenTree
{
GenTreePtr gtInlineCandidate;

GenTreePtr gtInlineCandidate;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here, and since there seems to be consensus on this, you could change GenTreePtr to GenTree*

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 26, 2017

Am also going to squash things down to one commit when merging...

Copy link

@JosephTremoulet JosephTremoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor comments.

// This helper call is marked as a "Special DCE" helper during
// importation, over in fgGetStaticsCCtorHelper.
//
// (2) NYI. If after, tunneling through GT_RET_VALs, we find that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that if the comma after GT_RET_VALs is there, then the first comma needs to be there too, just that it goes after "If" rather than "after" (since "after tunneling through GT_RET_VALs" is the dependent clause, modifying "find", that the commas set apart). But apparently I think that "if the comma after GT_RET_VALs is there" is somehow an exception to that rule...

@@ -6415,6 +6415,7 @@ GenTreeLclFld* Compiler::gtNewLclFldNode(unsigned lnum, var_types type, unsigned
}

GenTreePtr Compiler::gtNewInlineCandidateReturnExpr(GenTreePtr inlineCandidate, var_types type)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

//
// Look for the following tree shapes
// prejit: (IND (ADD (CONST, CALL(special dce helper...))))
// jit : (COMMA (CALL(special dce helper...), (FIELD ...)))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate that these have different patterns, and jit-diff only shows prejit diffs. Any thoughts about how to add regression testing for the jit case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could possibly catch regressions in a perf test. Or SPMI if we ever get that into CI.

// Expect one class generic parameter; figure out which it is.
//
// Probably should do this over on the runtime side now and just
// pass in the method handle.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you make this change? Create a follow-up issue? Revert this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should remove the comment.

Have some prospective follow on changes where I just have the type of T and not the method, and want to figure out the type of the default comparer. So having the jit interface go from type to type is more useful.

} CONTRACTL_END;

// Mirrors the logic in BCL's CompareHelpers.CreateDefaultEqualityComparer
// And in compile.cpp's SpecializeEqualityComparer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all three of these places have comments referencing the other two?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, will fix.

Copy link

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good, with some minor comments


// Special case for byte
//
// Using CLASS__BYTE here doesn't work, figure out why

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to leave this comment in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I'll remove it.

CorElementType normType = elemTypeHnd.GetVerifierCorElementType();

switch(normType)
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want reference or include this comment explaining why we have the switch cases below:

from BCL\System\Collections\Generic\EqualityComparer.cs

// Depending on the enum type, we need to special case the comparers so that we avoid boxing
// Note: We have different comparers for Short and SByte because for those types we need to make sure we call GetHashCode on the actual underlying type as the 
// implementation of GetHashCode is more complex than for the other types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a comment above indicating that the logic here must match what happens when you call CompareHelpers.CreateDefaultEqualityComparer. You think I need more emphasis on this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had trouble understanding why two cases were done separately and then four other cases were combined. It doesn't really make sense so some explanation is needed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enums are special creatures. I'll add a comment.

@AndyAyersMS
Copy link
Member Author

Perf test results (using same ValueTuple type as above):

Test Name Metric Iterations AVERAGE STDEV.S MIN MAX
Devirtualization.EqualityComparer.ValueTupleCompare Duration 16 652.090 10.632 645.172 687.140
Devirtualization.EqualityComparer.ValueTupleCompareCached Duration 11 963.699 5.058 955.513 973.128
Devirtualization.EqualityComparer.ValueTupleCompareNoOpt Duration 9 1142.399 5.125 1133.537 1149.070
Devirtualization.EqualityComparer.ValueTupleCompareWrapped Duration 10 1053.815 23.636 1039.294 1119.065

Formatting leg hit some kind of network timeout, retrying

@dotnet-bot retest Ubuntu x64 Formatting

@AndyAyersMS
Copy link
Member Author

Arm LB failure:

13:26:12 docker: Error response from daemon: failed to get network during CreateEndpoint: could not find endpoint count for network bridge: Key not found in store

retrying.

@dotnet-bot retest Ubuntu armlb Cross Release Build

@AndyAyersMS
Copy link
Member Author

Since there are now merge conflicts I'm going to rebase and squash locally, then force push.

Mark `EqualityComparer<T>.Default`'s getter as `[Intrinsic]` so
the jit knows there is something special about it. Extend the jit's
named intrinsic recognizer to recognize this method.

Add a new jit interface method to determine the exact type returned
by `EqualityComparer<T>.Default`, given `T`. Compute the return type by
mirroring the logic used in the actual implementation. Bail out when
`T` is not final as those cases won't simplify down much and lead to
code bloat.

Invoke this interface method when trying to devirtualize calls where
the 'this' object in the call comes from `EqualityComparer<T>.Default`.

The devirtualized methods can then be inlined. Since the specific comparer
`Equal` and `GetHashCode` methods look more complicated in IL than they
really are, mark them with AggressiveInlining attributes.

If devirtualization and inlining happen, it is quite likely that the value
of the comparer object itself is not used in the body of the comparer. This
value comes from a static field cache on the comparer helper.

When the comparer value is ignored, the jit removes the field access since it
is non-faulting. It also removes the the class init helper that is there to
ensure that the (no-longer accessed) field is properly initialized. This helper
has relatively high overhead even in the fast case where the class has been
initialized aready.

Add a perf test.

Closes #6688.
@AndyAyersMS AndyAyersMS force-pushed the EqualityComparerDefault branch from 0152adf to fb5edb0 Compare September 26, 2017 22:31
@AndyAyersMS
Copy link
Member Author

Arm failures are almost certainly some kind of infrastructure issue.

@AndyAyersMS
Copy link
Member Author

@dotnet/dnceng any idea what is going on with CoreCLR the arm test legs lately? They are all timing out.

@AndyAyersMS
Copy link
Member Author

One theory is that the test machines can get oversubscribed leading to timeouts. They seem to be idle now....

@dotnet-bot retest Windows_NT arm Cross Checked Build and Test
@dotnet-bot retest Windows_NT armlb Cross Checked Build and Test

@AndyAyersMS AndyAyersMS merged commit ccc5e17 into dotnet:master Sep 27, 2017
@AndyAyersMS AndyAyersMS deleted the EqualityComparerDefault branch September 27, 2017 14:15
@benaadams benaadams mentioned this pull request Dec 7, 2017
AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this pull request Nov 8, 2018
The jit incorporates the value of integer and float typed initonly static
fields into its codegen, if the class initializer has already run.

The jit can't incorporate the values of ref typed initonly static fields,
but the types of those values can't change, and the jit can use this knowledge
to enable type based optimizations like devirtualization.

In particular for static fields initialized by complex class factory logic the
jit can now see the end result of that logic instead of having to try and deduce
the type of object that will initialize or did initialize the field.

Examples of this factory pattern in include `EqualityComparer<T>.Default` and
`Comparer<T>.Default`. The former is already optimized in some cases by via
special-purpose modelling in the framework, jit, and runtime (see dotnet#14125) but
the latter is not. With this change calls through `Comparer<T>.Default` may now
also devirtualize (though won't yet inline as the devirtualization happens
late).

Addresses #4108.
AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this pull request Nov 8, 2018
The jit incorporates the value of integer and float typed initonly static
fields into its codegen, if the class initializer has already run.

The jit can't incorporate the values of ref typed initonly static fields,
but the types of those values can't change, and the jit can use this knowledge
to enable type based optimizations like devirtualization.

In particular for static fields initialized by complex class factory logic the
jit can now see the end result of that logic instead of having to try and deduce
the type of object that will initialize or did initialize the field.

Examples of this factory pattern in include `EqualityComparer<T>.Default` and
`Comparer<T>.Default`. The former is already optimized in some cases by via
special-purpose modelling in the framework, jit, and runtime (see dotnet#14125) but
the latter is not. With this change calls through `Comparer<T>.Default` may now
also devirtualize (though won't yet inline as the devirtualization happens
late).

Addresses #4108.
AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this pull request Nov 12, 2018
The jit incorporates the value of integer and float typed initonly static
fields into its codegen, if the class initializer has already run.

The jit can't incorporate the values of ref typed initonly static fields,
but the types of those values can't change, and the jit can use this knowledge
to enable type based optimizations like devirtualization.

In particular for static fields initialized by complex class factory logic the
jit can now see the end result of that logic instead of having to try and deduce
the type of object that will initialize or did initialize the field.

Examples of this factory pattern in include `EqualityComparer<T>.Default` and
`Comparer<T>.Default`. The former is already optimized in some cases by via
special-purpose modelling in the framework, jit, and runtime (see dotnet#14125) but
the latter is not. With this change calls through `Comparer<T>.Default` may now
also devirtualize (though won't yet inline as the devirtualization happens
late).

Addresses #4108.
AndyAyersMS added a commit that referenced this pull request Nov 12, 2018
The jit incorporates the value of integer and float typed initonly static
fields into its codegen, if the class initializer has already run.

The jit can't incorporate the values of ref typed initonly static fields,
but the types of those values can't change, and the jit can use this knowledge
to enable type based optimizations like devirtualization.

In particular for static fields initialized by complex class factory logic the
jit can now see the end result of that logic instead of having to try and deduce
the type of object that will initialize or did initialize the field.

Examples of this factory pattern in include `EqualityComparer<T>.Default` and
`Comparer<T>.Default`. The former is already optimized in some cases by via
special-purpose modelling in the framework, jit, and runtime (see #14125) but
the latter is not. With this change calls through `Comparer<T>.Default` may now
also devirtualize (though won't yet inline as the devirtualization happens
late).

Also update the reflection code to throw an exception instead of changing the value
of a fully initialized static readonly field.

Closes #4108.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…/coreclr#20886)

The jit incorporates the value of integer and float typed initonly static
fields into its codegen, if the class initializer has already run.

The jit can't incorporate the values of ref typed initonly static fields,
but the types of those values can't change, and the jit can use this knowledge
to enable type based optimizations like devirtualization.

In particular for static fields initialized by complex class factory logic the
jit can now see the end result of that logic instead of having to try and deduce
the type of object that will initialize or did initialize the field.

Examples of this factory pattern in include `EqualityComparer<T>.Default` and
`Comparer<T>.Default`. The former is already optimized in some cases by via
special-purpose modelling in the framework, jit, and runtime (see dotnet/coreclr#14125) but
the latter is not. With this change calls through `Comparer<T>.Default` may now
also devirtualize (though won't yet inline as the devirtualization happens
late).

Also update the reflection code to throw an exception instead of changing the value
of a fully initialized static readonly field.

Closes dotnet/coreclr#4108.


Commit migrated from dotnet/coreclr@c2abe89
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants