Allow jit to examine type of initonly static ref typed fields #20886
Conversation
src/vm/jitinterface.cpp
Outdated
| { | ||
| MethodTable* pEnclosingMT = field->GetEnclosingMethodTable(); | ||
|
|
||
| // We must not call here for statics of collectible types. |
There was a problem hiding this comment.
Why not? What guarantees it that you won't come here for collectible types?
There was a problem hiding this comment.
Not sure. This follows what we do with getFieldAddress so there is evidently some constraint on how the jit models more complex field accesses that leads us to never call back for these cases.
But probably better not to rely on this and just add the disqualifying check here.
There was a problem hiding this comment.
I do not think there needs to be disqualifying check on this path. This path should work just fine for fields on collectible types too.
The reason why getFieldAddress does not return addresses for collectible types is that the storage is not pinned for them, and so you cannot burn the address into the JITed code.
src/zap/zapinfo.h
Outdated
| @@ -417,6 +417,9 @@ class ZapInfo | |||
|
|
|||
| void * getFieldAddress(CORINFO_FIELD_HANDLE field, | |||
| void **ppIndirection); | |||
| CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, | |||
| bool* isInitO); | |||
src/vm/jitinterface.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // If the field is init only, and the class is known, the class can't ever change. |
There was a problem hiding this comment.
This is not sufficient condition. The static constructor has to be finished running for this to be true. Consider this:
using System;
class Test
{
static readonly object s;
static Test()
{
s = new object();
DoStuff();
s = "Method";
}
static void DoStuff()
{
Console.WriteLine(s.GetType());
}
static void Main()
{
}
}
There was a problem hiding this comment.
Good point -- is checking that pEnclosingMT->IsClassInited() sufficient?
There was a problem hiding this comment.
Yes, it should be. You may want to do it inside the block above only when you are returning the classhandle (e.g. IsClassInited does not work when pEnclosingMT is shared generic).
There was a problem hiding this comment.
Also, isInitOnly is not the best name of the flag once you fix this...
There was a problem hiding this comment.
bool *pIsFinal or bool *pIsImmutable?
Or flip this around and have bool *pIsSpeculative flag instead. When the EE returns *pIsSpeculative = true, it would mean that the answer is a guess that the JIT has to emit a test for. I think bool *pIsSpeculative would work well as a general pattern for other APIs (e.g. devirtualization). If the JIT is not interested in speculative answer, it would pass pIsSpeculative: NULL.
There was a problem hiding this comment.
Thanks. isSpeculative is a great suggestion. That's what I'll go with.
|
@jkotas, @erozenfeld PTAL Impact of this on codegen is a bit tricky to evaluate, but here's one take. Jit-diffs (in the new Most of the regressions here are "reconciliation" issues in corelib, where we see different methods get jitted. Almost certainly a testing artifact, though something I need to look at more closely. Improvements are seen for fields like current culture and text encoder, some specialized type comparisons, and number of cases where we already were devirtualizing but can now also omit a null check. Also makes some headway into the patterns for the test case in #17273 that we don't optimize very well; eg in ;; before
mov rax, qword ptr [rcx]
mov rax, qword ptr [rax+64]
call qword ptr [rax+32]EqualityComparer`1:Equals(ref,ref):bool:this
;; after
call GenericEqualityComparer`1:Equals(ref,ref):bool:this |
|
The (apparently new) |
|
Yeah that failure is a bug in Azure DevOps that causes the artifact publishing to fail on PR builds from forks. I think they've known about it for a few weeks now. |
|
Ubuntu failures look like odd glitches: Arm failure was a disconnect: Am about to push an update so won't rerun. |
|
@dotnet/jit-contrib one thing I should highlight. Because the values returned by this new jit interface method can change over time (even in the non-speculative case), it creates a problem for SPMI. I haven't looked at SPMI in detail but my guess is that only the last of these answers will survive in the results cache. So the SPMI replay may not be completely faithful recreation of the jit-ee traffic. I thought about caching the result of this query on the jit side on the root compiler, so that from the jit perspective the answer wouldn't change in the middle of jitting a method, but haven't done that. So it's possible in a method with a lot of field accesses that they get optimized differently if a jitting thread is racing with an initializing thread. Note we have this situation already with int and float typed initonly statics. |
And number of other cases. For example, you can get different answers for whether the constructor triggers are required over time. |
|
Reran PMI and suspect my previous results had version skew or something (hence all the unique to diff methods). New results: |
src/vm/jitinterface.cpp
Outdated
| // Figure out what to report back. | ||
| DWORD fieldAttribs = field->GetAttributes(); | ||
| bool isInitOnly = !!IsFdInitOnly(fieldAttribs); | ||
| bool isResultImmutable = isInitOnly && isClassInitialized; |
There was a problem hiding this comment.
bool isResultImmutable = isClassInitialized && IsFdInitOnly(field->GetAttributes()) to make this cheaper when the class is not initialized? field->GetAttributes() is not super cheap.
src/jit/gentree.cpp
Outdated
| { | ||
| objClass = fieldClass; | ||
| } | ||
| objClass = gtGetFieldClassHandle(fieldHnd, isExact, isNonNull); |
There was a problem hiding this comment.
I realize that the isExact variable was pre-existing, but it tripped me up here, because it appears to be a bool rather than a bool *. Might be nice to rename it something like pIsExact.
There was a problem hiding this comment.
Seems reasonable.
Also I was somewhat surprised to see in an earlier iteration that VC++ allows isExact = false to compile. Clang doesn't though....
| { | ||
| MethodTable *pObjMT = fieldObj->GetMethodTable(); | ||
|
|
||
| // TODO: Check if the jit is allowed to embed this handle in jitted code. |
There was a problem hiding this comment.
What is the remaining issue beyond the things already checked for?
There was a problem hiding this comment.
My understanding is that for collectible classes the runtime data structures may get recycled when the class is collected, so we need to be careful about making direct references to them from jitted code.
|
@AndyAyersMS Are you also going to look into adding the checks into reflection to prevent it from mutating static readonly fields that we have discussed offline? (I can help you with that if needed.) |
|
@jkotas yes I'll add the checks. Pointers welcome. |
|
|
@jkotas tale a look at the most recent commit; hopefully this is in the right neighborhood. It seems to work on my simple test case for |
| @@ -337,6 +337,15 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob | |||
|
|
|||
| FieldDesc* pFieldDesc = gc.refField->GetField(); | |||
|
|
|||
| // Verify we're not trying to set the value of a static initonly field | |||
There was a problem hiding this comment.
When you have a choice, it is better to have code inside HELPER_METHOD_FRAME_BEGIN_PROTECT. It reduces chances of GC holes. (You will need to use COMPlusThrow once you do that.)
src/vm/reflectioninvocation.cpp
Outdated
| @@ -1714,6 +1723,11 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA | |||
| if (IsFdLiteral(attr)) | |||
There was a problem hiding this comment.
You can delete this check while you are on it. The type loader will not create field descs for IsFdLiteral fields.
src/vm/reflectioninvocation.cpp
Outdated
| { | ||
| MethodTable* pEnclosingMT = pFieldDesc->GetEnclosingMethodTable(); | ||
| if (pEnclosingMT->IsClassInited() && IsFdInitOnly(pFieldDesc->GetAttributes())) | ||
| FCThrowVoid(kFieldAccessException); |
|
Added initial cut at an error message string. Test case now returns: Should I add this test case somewhere under |
|
CoreFx test is trying to set a static readonly field and failing... |
|
The compiled version of the test didn't fail, so jitted code is able to alter the field still. |
src/vm/reflectioninvocation.cpp
Outdated
| if (pEnclosingMT->IsClassInited() && IsFdInitOnly(pFieldDesc->GetAttributes())) | ||
| { | ||
| DefineFullyQualifiedNameForClassW(); | ||
| StackSString ssFieldName(SString::Utf8, pFieldDesc->GetName()); |
There was a problem hiding this comment.
This can be just SString. StackSString is an optimization - no need to optimize exception throwing paths.
This is bad test. I would disable it in https://github.com/dotnet/coreclr/blob/master/tests/CoreFX/CoreFX.issues.json and delete in CoreFX.
The invalid expressions tends to have different behavior between interpretted and compiled mode. It is close to impossible to make this check 100% bullet proof. There are many (unsafe) ways to get the readonly field and modify it. The reflection check should catch the most common cases.
Yes. (Alternatively, it can be also in corefx repo. I think having low-level tests like this in CoreCLR is fine.)
I do not think you need to write it in IL. The error handling for SetValueDirect is bogus. The TypedReference can point to anything for static fields, it just cannot be null: public class Program
{
public static readonly string s = "Hello";
static void Main()
{
int i = 0;
typeof(Program).GetField("s").SetValueDirect(__makeref(i), "World");
Console.WriteLine(s);
}
} |
|
@dotnet/dnceng ci.dot.net seems to be unreachable |
|
CoreFX failure in |
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.
Also remove assert for collectible classes and fix typo.
Flip the sense of the return flag for getStaticFieldCurrentClass and update remainder of code accordingly. Change jit to not ask for speculative results.
Rename bool out parameters to have `p` prefix for clarity. Defer fetching field attributes if class is not initialized.
AndyAyersMS
force-pushed
the
GetExactTypeForRefClassReadonlyStatics
branch
from
31ed3ea
to
bf3c1a5
Compare
|
I've removed the test that was setting a static readonly value via reflection in dotnet/corefx#33413 so the new exclusion won't be needed for long... |
…serializers. Make serializers abstract classes instead of interfaces. Now nested tuples perf is c.5-10x faster than JSON and profiler is almost clean. # Bench **`((((int, int), (int, int)), ((int, int), (int, int))), (((int, int), (int, int)), ((int, int), (int, int))))`** Binary: 3.86 Mops JSON: 0.85 Mops **`(((int, int), (int, int)), ((int, int), (int, int)))`** Binary: 7.75 Mops JSON: 1.60 Mops **`((int, int), (int, int))`** Binary: 20.0 Mops JSON: 2.60 Mops **`(int, int)`** Binary: 138.0 Mops * not nested same type is extremely fast JSON: 4.20 Mops **`(int, long)`** Binary: 27.0 Mops JSON: 3.90 Mops Utf8Json could use same technique for formatters, but that's a lot of stupid refactoring work and should be done in a distant future at least after System.Text.Json is shipped. dotnet/coreclr#20886
richlander
added
the
breaking-change
…/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
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>.DefaultandComparer<T>.Default. The former is already optimized in some cases by viaspecial-purpose modelling in the framework, jit, and runtime (see #14125) but
the latter is not. With this change calls through
Comparer<T>.Defaultmay nowalso devirtualize (though won't yet inline as the devirtualization happens
late).
Addresses #4108.