Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend PreInitialization Support to readonly GC fields #84431

Closed
wants to merge 3 commits into from
Closed

Extend PreInitialization Support to readonly GC fields #84431

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a53219
Extend PreInitialization Support to readonly GC fields
Suchiman Apr 6, 2023
da9c26b
Support simple function pointer fields
Suchiman Apr 10, 2023
842916f
Offset based symbolic GC values + initobj
Suchiman Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 186 additions & 25 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Runtime.InteropServices;

using ILCompiler.DependencyAnalysis;
Expand Down Expand Up @@ -49,7 +50,7 @@ private TypePreinit(MetadataType owningType, CompilationModuleGroup compilationG
if (!field.IsStatic || field.IsLiteral || field.IsThreadStatic || field.HasRva)
continue;

_fieldValues.Add(field, NewUninitializedLocationValue(field.FieldType));
_fieldValues.Add(field, NewUninitializedLocationValue(field.FieldType));
}
}

Expand Down Expand Up @@ -352,11 +353,11 @@ private Status TryScanMethod(MethodIL methodIL, Value[] parameters, Stack<Method
// and resetting it would lead to unpredictable analysis durations.
int baseInstructionCounter = instructionCounter;
Status status = nestedPreinit.TryScanMethod(field.OwningType.GetStaticConstructor(), null, recursionProtect, ref instructionCounter, out Value _);
recursionProtect.Pop();
if (!status.IsSuccessful)
{
return Status.Fail(methodIL.OwningMethod, opcode, "Nested cctor failed to preinit");
}
recursionProtect.Pop();
Value value = nestedPreinit._fieldValues[field];
if (value is ValueTypeValue)
stack.PushFromLocation(field.FieldType, value);
Expand Down Expand Up @@ -440,12 +441,11 @@ private Status TryScanMethod(MethodIL methodIL, Value[] parameters, Stack<Method
recursionProtect ??= new Stack<MethodDesc>();
recursionProtect.Push(methodIL.OwningMethod);
Status callResult = TryScanMethod(method, methodParams, recursionProtect, ref instructionCounter, out retVal);
recursionProtect.Pop();
if (!callResult.IsSuccessful)
{
recursionProtect.Pop();
return callResult;
}
recursionProtect.Pop();
}

if (!methodSig.ReturnType.IsVoid)
Expand Down Expand Up @@ -575,13 +575,11 @@ private Status TryScanMethod(MethodIL methodIL, Value[] parameters, Stack<Method
recursionProtect ??= new Stack<MethodDesc>();
recursionProtect.Push(methodIL.OwningMethod);
Status ctorCallResult = TryScanMethod(ctor, ctorParameters, recursionProtect, ref instructionCounter, out _);
recursionProtect.Pop();
if (!ctorCallResult.IsSuccessful)
{
recursionProtect.Pop();
return ctorCallResult;
}

recursionProtect.Pop();
}

stack.PushFromLocation(owningType, instance);
Expand All @@ -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)
Copy link
Member

@jkotas jkotas Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IsInitOnly on instance fields does not provide useful invariants for codegen purposes. Instance IsInitOnly fields are mutable via reflection or via unsafe code.

Copy link
Member

Choose a reason for hiding this comment

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

Can this be extended to any instance GC fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// We don't want to end up with GC pointers in the frozen region
// because write barriers can't handle that.
// We can make an exception for readonly fields.

Copy link
Member

Choose a reason for hiding this comment

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

This exception can be only made for static readonly fields that are actually immutable. Instance readonly fields are not actually immutable.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code as of this PR was able to preinit the following in a hello world repro project https://gist.github.com/Suchiman/22f9d5ebf58a820456986b93accc3af2

Copy link
Contributor Author

@Suchiman Suchiman Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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>'

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
return Status.Fail(methodIL.OwningMethod, opcode, "Reference field");
}
Expand Down Expand Up @@ -1891,8 +1889,9 @@ private abstract class BaseValueTypeValue : Value
}

// Also represents pointers and function pointer.
private sealed class ValueTypeValue : BaseValueTypeValue, IAssignableValue
private sealed class ValueTypeValue : BaseValueTypeValue, IAssignableValue, IHasInstanceFields
{
public Dictionary<FieldDesc, Value> GCFields;
public readonly byte[] InstanceBytes;

public override int Size => InstanceBytes.Length;
Expand All @@ -1903,19 +1902,20 @@ public ValueTypeValue(TypeDesc type)
InstanceBytes = new byte[type.GetElementSize().AsInt];
}

private ValueTypeValue(byte[] bytes)
private ValueTypeValue(byte[] bytes, Dictionary<FieldDesc, Value> gcFields = null)
{
InstanceBytes = bytes;
GCFields = gcFields;
}

public override Value Clone()
{
return new ValueTypeValue((byte[])InstanceBytes.Clone());
return new ValueTypeValue((byte[])InstanceBytes.Clone(), GCFields == null ? null : new(GCFields));
}

public override bool TryCreateByRef(out Value value)
{
value = new ByRefValue(InstanceBytes, 0);
value = new ByRefValue(InstanceBytes, 0, this);
return true;
}

Expand All @@ -1933,6 +1933,7 @@ bool IAssignableValue.TryAssign(Value value)
}

Array.Copy(vtvalue.InstanceBytes, InstanceBytes, InstanceBytes.Length);
GCFields = vtvalue.GCFields == null ? null : new(vtvalue.GCFields);
return true;
}

Expand All @@ -1942,24 +1943,69 @@ public override bool Equals(Value value)
|| vtvalue.InstanceBytes.Length != InstanceBytes.Length)
{
ThrowHelper.ThrowInvalidProgramException();
return false;
}

if ((GCFields == null) != (vtvalue.GCFields == null) || GCFields != null && GCFields.Count != vtvalue.GCFields.Count)
return false;

for (int i = 0; i < InstanceBytes.Length; i++)
{
if (InstanceBytes[i] != ((ValueTypeValue)value).InstanceBytes[i])
if (InstanceBytes[i] != vtvalue.InstanceBytes[i])
return false;
}

if (GCFields != null)
{
foreach (var (field, myValue) in GCFields)
{
if (!vtvalue.GCFields.TryGetValue(field, out var otherValue))
return false;

if ((myValue == null) != (otherValue == null) || myValue != null && !myValue.Equals(otherValue))
return false;
}
}

return true;
}

public override void WriteFieldData(ref ObjectDataBuilder builder, NodeFactory factory)
{
builder.EmitBytes(InstanceBytes);
if (GCFields != null)
{
int bytesWritten = 0;
foreach (var (field, value) in GCFields.OrderBy(x => x.Key.Offset.AsInt))
{
int fieldOffset = field.Offset.AsInt;
int fieldSize = field.FieldType.GetElementSize().AsInt;
//if (fieldOffset + fieldSize > _instanceBytes.Length - _offset)
// ThrowHelper.ThrowInvalidProgramException();

int bytesFromInstance = bytesWritten - fieldOffset;
if (bytesFromInstance > 0)
{
builder.EmitBytes(InstanceBytes, bytesWritten, bytesFromInstance);
bytesWritten += bytesFromInstance;
}
value.WriteFieldData(ref builder, factory);
bytesWritten += fieldSize;
}
}
else
{
builder.EmitBytes(InstanceBytes);
}
}

public override bool GetRawData(NodeFactory factory, out object data)
{
if (GCFields != null)
{
data = null;
return false;
}

data = InstanceBytes;
return true;
}
Expand All @@ -1973,6 +2019,33 @@ private byte[] AsExactByteCount(int size)
return InstanceBytes;
}

Value IHasInstanceFields.GetField(FieldDesc field)
{
if (field.FieldType.IsGCPointer)
{
return GCFields.GetValueOrDefault(field);
}
else
{
return new FieldAccessor(InstanceBytes, 0).GetField(field);
}
}

void IHasInstanceFields.SetField(FieldDesc field, Value value)
{
if (field.FieldType.IsGCPointer)
{
GCFields ??= new();
GCFields[field] = value;
}
else
{
new FieldAccessor(InstanceBytes, 0).SetField(field, value);
}
}

ByRefValue IHasInstanceFields.GetFieldAddress(FieldDesc field) => new FieldAccessor(InstanceBytes, 0).GetFieldAddress(field);

public override sbyte AsSByte() => (sbyte)AsExactByteCount(1)[0];
public override short AsInt16() => BitConverter.ToInt16(AsExactByteCount(2), 0);
public override int AsInt32() => BitConverter.ToInt32(AsExactByteCount(4), 0);
Expand Down Expand Up @@ -2144,11 +2217,13 @@ private sealed class ByRefValue : Value, IHasInstanceFields
{
public readonly byte[] PointedToBytes;
public readonly int PointedToOffset;
public readonly IHasInstanceFields PointedToType;

public ByRefValue(byte[] pointedToBytes, int pointedToOffset)
public ByRefValue(byte[] pointedToBytes, int pointedToOffset, IHasInstanceFields pointedToType = null)
{
PointedToBytes = pointedToBytes;
PointedToOffset = pointedToOffset;
PointedToType = pointedToType;
}

public override bool Equals(Value value)
Expand All @@ -2162,9 +2237,39 @@ public override bool Equals(Value value)
&& PointedToOffset == ((ByRefValue)value).PointedToOffset;
}

Value IHasInstanceFields.GetField(FieldDesc field) => new FieldAccessor(PointedToBytes, PointedToOffset).GetField(field);
void IHasInstanceFields.SetField(FieldDesc field, Value value) => new FieldAccessor(PointedToBytes, PointedToOffset).SetField(field, value);
ByRefValue IHasInstanceFields.GetFieldAddress(FieldDesc field) => new FieldAccessor(PointedToBytes, PointedToOffset).GetFieldAddress(field);
Value IHasInstanceFields.GetField(FieldDesc field)
{
if (PointedToType != null)
{
return PointedToType.GetField(field);
}
else
{
return new FieldAccessor(PointedToBytes, PointedToOffset).GetField(field);
}
}

void IHasInstanceFields.SetField(FieldDesc field, Value value)
{
if (PointedToType != null)
{
PointedToType.SetField(field, value);
}
else
{
new FieldAccessor(PointedToBytes, PointedToOffset).SetField(field, value);
}
}

ByRefValue IHasInstanceFields.GetFieldAddress(FieldDesc field)
{
if (field.FieldType.IsGCPointer)
{
throw new NotSupportedException();
}

return new FieldAccessor(PointedToBytes, PointedToOffset).GetFieldAddress(field);
}

public void Initialize(int size)
{
Expand Down Expand Up @@ -2523,6 +2628,7 @@ private class ObjectInstance : AllocatedReferenceTypeValue, IHasInstanceFields,
#pragma warning restore CA1852
{
private readonly byte[] _data;
public Dictionary<FieldDesc, Value> GCFields;

public ObjectInstance(DefType type, AllocationSite allocationSite)
: base(type, allocationSite)
Expand Down Expand Up @@ -2565,9 +2671,32 @@ public bool TryUnboxAny(TypeDesc type, out Value value)
return true;
}

Value IHasInstanceFields.GetField(FieldDesc field) => new FieldAccessor(_data).GetField(field);
void IHasInstanceFields.SetField(FieldDesc field, Value value) => new FieldAccessor(_data).SetField(field, value);
ByRefValue IHasInstanceFields.GetFieldAddress(FieldDesc field) => new FieldAccessor(_data).GetFieldAddress(field);
Value IHasInstanceFields.GetField(FieldDesc field)
{
if (field.FieldType.IsGCPointer)
{
return GCFields.GetValueOrDefault(field);
}
else
{
return new FieldAccessor(_data, 0).GetField(field);
}
}

void IHasInstanceFields.SetField(FieldDesc field, Value value)
{
if (field.FieldType.IsGCPointer)
{
GCFields ??= new();
GCFields[field] = value;
}
else
{
new FieldAccessor(_data, 0).SetField(field, value);
}
}

ByRefValue IHasInstanceFields.GetFieldAddress(FieldDesc field) => new FieldAccessor(_data, 0).GetFieldAddress(field);

public override void WriteFieldData(ref ObjectDataBuilder builder, NodeFactory factory)
{
Expand All @@ -2581,10 +2710,42 @@ public virtual void WriteContent(ref ObjectDataBuilder builder, ISymbolNode this
Debug.Assert(!node.RepresentsIndirectionCell); // Shouldn't have allowed preinitializing this
builder.EmitPointerReloc(node);

// We skip the first pointer because that's the MethodTable pointer
// we just initialized above.
int pointerSize = factory.Target.PointerSize;
builder.EmitBytes(_data, pointerSize, _data.Length - pointerSize);
if (GCFields != null)
{
// We skip the first pointer because that's the MethodTable pointer
// we just initialized above.
int bytesWritten = factory.Target.PointerSize;
foreach (var (field, value) in GCFields.OrderBy(x => x.Key.Offset.AsInt))
{
int fieldOffset = field.Offset.AsInt;
int fieldSize = field.FieldType.GetElementSize().AsInt;
//if (fieldOffset + fieldSize > _instanceBytes.Length - _offset)
// ThrowHelper.ThrowInvalidProgramException();

int bytesFromInstance = bytesWritten - fieldOffset;
if (bytesFromInstance > 0)
{
builder.EmitBytes(_data, bytesWritten, bytesFromInstance);
bytesWritten += bytesFromInstance;
}
if (value == null)
{
builder.EmitZeroPointer();
}
else
{
value.WriteFieldData(ref builder, factory);
}
bytesWritten += fieldSize;
}
}
else
{
// We skip the first pointer because that's the MethodTable pointer
// we just initialized above.
int pointerSize = factory.Target.PointerSize;
builder.EmitBytes(_data, pointerSize, _data.Length - pointerSize);
}
}

public bool IsKnownImmutable => !Type.GetFields().GetEnumerator().MoveNext();
Expand Down