Skip to content

Allow preinitializing RuntimeType instances #94405

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

Merged
MichalStrehovsky merged 2 commits into from
Nov 13, 2023
Merged

Allow preinitializing RuntimeType instances #94405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6343d5c
Allow preinitializing RuntimeType instances
MichalStrehovsky Nov 6, 2023
05ad0f4
Update src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
jkotas Nov 10, 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
2 changes: 1 addition & 1 deletion src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8989,6 +8989,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
FALLTHROUGH;
case CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER:
case CORINFO_FIELD_STATIC_ADDRESS:
case CORINFO_FIELD_STATIC_RELOCATABLE:
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Nov 6, 2023

Choose a reason for hiding this comment

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

@EgorBo could you have a look at this? This affects a lot more than just RuntimeType and I might be missing why we weren't doing this before. (We might also want to do this for STATIC_RVA_ADDRESS.)

Copy link
Member

@EgorBo EgorBo Nov 6, 2023

Choose a reason for hiding this comment

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

@MichalStrehovsky wow, it's actually a good catch! It should improve some things, e.g.:

static readonly string Fld = "Hello";

[MethodImpl(MethodImplOptions.NoInlining)]
static string Test() => Fld;
; Assembly listing for method Benchmarks:Test():System.String (FullOpts)
-      mov      rax, qword ptr [(reloc 0x400000000042c388)]
-      mov      rax, gword ptr [rax+0x08]
+      lea      rax, gword ptr [(reloc 0x400000000042c398)]      ; '"Hello"'
       ret      
; Total bytes of code 12

Copy link
Member

@EgorBo EgorBo Nov 6, 2023

Choose a reason for hiding this comment

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

Basically, static readonly wasn't foldable for gc types for NAOT

// Replace static read-only fields with constant if possible
if ((aflags & CORINFO_ACCESS_GET) && (fieldInfo.fieldFlags & CORINFO_FLG_FIELD_FINAL))
{
Expand All @@ -9005,7 +9006,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CORINFO_FIELD_STATIC_RVA_ADDRESS:
case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER:
case CORINFO_FIELD_STATIC_READYTORUN_HELPER:
case CORINFO_FIELD_STATIC_RELOCATABLE:
op1 = impImportStaticFieldAddress(&resolvedToken, (CORINFO_ACCESS_FLAGS)aflags, &fieldInfo,
lclTyp, &indirFlags);
break;
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,13 @@ public SerializedFrozenObjectNode SerializedFrozenObject(MetadataType owningType
return _frozenObjectNodes.GetOrAdd(new SerializedFrozenObjectKey(owningType, allocationSiteId, data));
}

public FrozenRuntimeTypeNode SerializedMaximallyConstructableRuntimeTypeObject(TypeDesc type)
{
if (ConstructedEETypeNode.CreationAllowed(type))
return SerializedConstructedRuntimeTypeObject(type);
return SerializedNecessaryRuntimeTypeObject(type);
}

private NodeCache<TypeDesc, FrozenRuntimeTypeNode> _frozenConstructedRuntimeTypeNodes;

public FrozenRuntimeTypeNode SerializedConstructedRuntimeTypeObject(TypeDesc type)
Expand Down
38 changes: 28 additions & 10 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 @@ -375,7 +375,7 @@ private Status TryScanMethod(MethodIL methodIL, Value[] parameters, Stack<Method
if (value is ValueTypeValue)
stack.PushFromLocation(field.FieldType, value);
else if (value is ReferenceTypeValue referenceType)
stack.PushFromLocation(field.FieldType, referenceType.ToForeignInstance(baseInstructionCounter));
stack.PushFromLocation(field.FieldType, referenceType.ToForeignInstance(baseInstructionCounter, this));
else
return Status.Fail(methodIL.OwningMethod, opcode);
}
Expand Down Expand Up @@ -2388,7 +2388,7 @@ public override bool GetRawData(NodeFactory factory, out object data)
}
}

private sealed class RuntimeTypeValue : ReferenceTypeValue, IInternalModelingOnlyValue
private sealed class RuntimeTypeValue : ReferenceTypeValue
{
public TypeDesc TypeRepresented { get; }

Expand All @@ -2400,11 +2400,21 @@ public RuntimeTypeValue(TypeDesc type)

public override bool GetRawData(NodeFactory factory, out object data)
{
data = null;
return false;
data = factory.SerializedMaximallyConstructableRuntimeTypeObject(TypeRepresented);
return true;
}
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext)
{
if (!preinitContext._internedTypes.TryGetValue(TypeRepresented, out RuntimeTypeValue result))
{
preinitContext._internedTypes.Add(TypeRepresented, result = new RuntimeTypeValue(TypeRepresented));
}
return result;
}
public override void WriteFieldData(ref ObjectDataBuilder builder, NodeFactory factory)
{
builder.EmitPointerReloc(factory.SerializedMaximallyConstructableRuntimeTypeObject(TypeRepresented));
}
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter) => this;
public override void WriteFieldData(ref ObjectDataBuilder builder, NodeFactory factory) => throw new NotImplementedException();
}

private sealed class ReadOnlySpanValue : BaseValueTypeValue, IInternalModelingOnlyValue
Expand Down Expand Up @@ -2641,7 +2651,7 @@ public override bool Equals(Value value)
return this == value;
}

public abstract ReferenceTypeValue ToForeignInstance(int baseInstructionCounter);
public abstract ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext);
}

private struct AllocationSite
Expand Down Expand Up @@ -2669,7 +2679,7 @@ public AllocatedReferenceTypeValue(TypeDesc type, AllocationSite allocationSite)
AllocationSite = allocationSite;
}

public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter) =>
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext) =>
new ForeignTypeInstance(
Type,
new AllocationSite(AllocationSite.OwningType, AllocationSite.InstructionCounter - baseInstructionCounter),
Expand Down Expand Up @@ -2889,7 +2899,7 @@ public override void WriteFieldData(ref ObjectDataBuilder builder, NodeFactory f
}
}

public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter) => this;
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext) => this;
}

private sealed class StringInstance : ReferenceTypeValue, IHasInstanceFields
Expand Down Expand Up @@ -2947,7 +2957,15 @@ public override bool GetRawData(NodeFactory factory, out object data)
return true;
}

public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter) => this;
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext)
{
string value = ValueAsString;
if (!preinitContext._internedStrings.TryGetValue(value, out StringInstance result))
{
preinitContext._internedStrings.Add(value, result = new StringInstance(Type, value));
}
return result;
}
Value IHasInstanceFields.GetField(FieldDesc field) => new FieldAccessor(_value).GetField(field);
void IHasInstanceFields.SetField(FieldDesc field, Value value) => ThrowHelper.ThrowInvalidProgramException();
ByRefValue IHasInstanceFields.GetFieldAddress(FieldDesc field) => new FieldAccessor(_value).GetFieldAddress(field);
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ public FrozenRuntimeTypeNode NecessaryRuntimeTypeIfPossible(TypeDesc type)
{
bool canPotentiallyConstruct = _devirtualizationManager == null
? true : _devirtualizationManager.CanConstructType(type);
if (canPotentiallyConstruct && ConstructedEETypeNode.CreationAllowed(type))
return _nodeFactory.SerializedConstructedRuntimeTypeObject(type);
if (canPotentiallyConstruct)
return _nodeFactory.SerializedMaximallyConstructableRuntimeTypeObject(type);

return _nodeFactory.SerializedNecessaryRuntimeTypeObject(type);
}
Expand Down
6 changes: 4 additions & 2 deletions src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,13 +657,15 @@ class OtherClass
static int s_intValue = OtherClass.IntValue;
static string s_stringValue = OtherClass.StringValue;
static object s_objectValue = OtherClass.ObjectValue;
static bool s_areStringsSame = Object.ReferenceEquals(OtherClass.StringValue, "Hello");

public static void Run()
{
Assert.IsPreinitialized(typeof(TestInitFromOtherClass));
Assert.AreEqual(OtherClass.IntValue, s_intValue);
Assert.AreSame(OtherClass.StringValue, s_stringValue);
Assert.AreSame(OtherClass.ObjectValue, s_objectValue);
Assert.True(s_areStringsSame);
}
}

Expand Down Expand Up @@ -1282,8 +1284,8 @@ public static void Run()
Assert.True(!Foo<bool>.IsChar);
Assert.True(Foo<bool>.IsBool);

Assert.IsLazyInitialized(typeof(CharHolder));
Assert.IsLazyInitialized(typeof(IsChar));
Assert.IsPreinitialized(typeof(CharHolder));
Assert.IsPreinitialized(typeof(IsChar));
Assert.True(IsChar.Is);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,8 @@ class C3<T> : IInterface, IInterface<T>
static IInterface<object> s_c3a = new C3<object>();
static IInterface s_c3b = new C3<object>();

// Works around https://github.com/dotnet/runtime/issues/94399
[MethodImpl(MethodImplOptions.NoOptimization)]
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Nov 6, 2023

Choose a reason for hiding this comment

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

This found a new bug. The Run method is essentially optimized to a throw. It's ridiculous how well this optimizes:

  • We figured out the value assigned to the static is read only using whole program view (the static is not marked read only in source code)
  • RyuJIT then figured out exact type information from the preinit data blob.
  • RyuJIT then devirtualized the call (into the wrong method, but that's the bug)
  • RyuJIT then folded the string constant comparison.

All that's left is the throw (and a couple garbage instructions I couldn't fully make sense of).

public static void Run()
{
if (s_c1.Method(null) != "Method(object)")
Expand Down