Extend PreInitialization Support to readonly GC fields #84431
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue Details@MichalStrehovsky do you think the approach with the
|
| @@ -600,7 +598,7 @@ private Status TryScanMethod(MethodIL methodIL, Value[] parameters, Stack<Method | |||
| Value value = stack.PopIntoLocation(field.FieldType); | |||
| StackEntry instance = stack.Pop(); | |||
|
|
|||
| if (field.FieldType.IsGCPointer && value != null) | |||
| if (field.FieldType.IsGCPointer && value != null && !field.IsInitOnly) | |||
There was a problem hiding this comment.
IsInitOnly on instance fields does not provide useful invariants for codegen purposes. Instance IsInitOnly fields are mutable via reflection or via unsafe code.
There was a problem hiding this comment.
Can this be extended to any instance GC fields?
There was a problem hiding this comment.
The idea behind this was to respect the following comment:
runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs
Lines 541 to 544 in 6a53219
There was a problem hiding this comment.
This exception can be only made for static readonly fields that are actually immutable. Instance readonly fields are not actually immutable.
There was a problem hiding this comment.
Yeah, the exception wasn't particularly sound but it felt like a compromise we could make because it helped a lot at that time. In practice reflection-setting readonly fields rarely happens outside deserialization. I originally considered extending the check to only allow this for fields that are reflection-blocked but hit layering issues that I didn't want to get into. It was basically specially crafted to allow preinit of array enumerators.
Array enumerators got more broken recently (#82993). We'll have to live with them not being preinitialized so if we don't find a workable solution for this PR, I'll submit a PR to delete the exception because it no longer serves the intended purpose and can only cause trouble.
Do you have any numbers on how many more things we can preinitialize with this? The compiler will dump them if you pass --verbose so a before/after of that list could inform how much more value this is adding.
The features in the interpreter were basically driven by what gives the best bang for the buck.
There was a problem hiding this comment.
The code as of this PR was able to preinit the following in a hello world repro project https://gist.github.com/Suchiman/22f9d5ebf58a820456986b93accc3af2
There was a problem hiding this comment.
Here's a comparison of before and after this change for a Hello World https://gist.github.com/Suchiman/bd346b21b0f5f7498c6780d6dddcaca7
The TL;DR; seems to be that from the 104 additional types to be preinitialized, there's 100xSZGenericArrayEnumerator and
ILC: Preinitialized type '[S.P.CoreLib]System.Collections.Generic.NonRandomizedStringEqualityComparer'
ILC: Preinitialized type '[S.P.CoreLib]System.Collections.ObjectModel.ReadOnlyCollection`1<System.Reflection.CustomAttributeData>'
ILC: Preinitialized type '[S.P.CoreLib]System.Collections.ObjectModel.ReadOnlyCollection`1<System.Reflection.CustomAttributeNamedArgument>'
ILC: Preinitialized type '[S.P.CoreLib]System.Collections.ObjectModel.ReadOnlyCollection`1<System.Reflection.CustomAttributeTypedArgument>'
There was a problem hiding this comment.
Thanks! We can basically look at this as a potential fix for #82993. The 4 extra types are a nice bonus, but maybe wouldn't be enough to motivate this change if we look at the complexity vs benefits (I think we could find less scary things to implement that will preinitialize extra 4 type; by "less scary" I mean things that don't affect the memory model of the interpreter. When I wrote on discord that I'm now afraid to touch it, I meant that I don't remember the places that may rely on invariants that would no longer hold after this change.)
I was honestly thinking of fixing #82993 by #ifdefing stuff touched in #82751 to make things look like before the change on the NativeAOT side instead of beefing up the preinitializer.
It might take a while until I can review this change - the thing is that I'd need to review the whole interpreter to ensure we don't break invariants that the rest assumes. I never expected we'd be doing stores into reference fields.
ghost
closed this
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
@MichalStrehovsky do you think the approach with the
Dictionary<FieldDesc, Value> GCFields;overlaying theInstanceBytesis viable?fixes #82993
fixes #83043
TODO:
Assert.IsLazyInitializedalways fails in Preinitialization tests (even if ILC claimed it did not initialize that class)