Skip to content

CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

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
VSadov merged 30 commits into from
Mar 16, 2023
Merged

CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
da7b00b
initial port from the prototype
VSadov Feb 25, 2023
170a306
parse the attribute in MT builder
VSadov Feb 26, 2023
bb1c3db
validate replicated size
VSadov Feb 26, 2023
0a4cd78
aot changes
VSadov Feb 26, 2023
6eeb887
refmap
VSadov Feb 27, 2023
f0d156b
validate total size in ilc
VSadov Feb 27, 2023
470aee2
add the actual attribute
VSadov Feb 28, 2023
78ab523
Apply suggestions from code review
VSadov Feb 28, 2023
a452be7
add a small use of InlineArray in CoreLib - to get some crossgen cove…
VSadov Feb 28, 2023
2801f50
a few more uses in CorLib
VSadov Mar 1, 2023
e3284d1
fix for sequential layout
VSadov Mar 1, 2023
4ff0a25
Standardize on use of "InlineArray" in the implementation
VSadov Mar 2, 2023
b406dfe
simpler layout replication
VSadov Mar 2, 2023
7f197d6
some initial tests (will add more)
VSadov Mar 3, 2023
1852993
more tests
VSadov Mar 4, 2023
295caf1
limit the max size of array instance to 1MiB
VSadov Mar 4, 2023
e7738ea
fix an assert in importercalls.cpp
VSadov Mar 4, 2023
065467b
"result" in GC layout should track the pointer count, not the size
VSadov Mar 4, 2023
62b20c8
error messages
VSadov Mar 8, 2023
01e2e17
fixed uses of "value array" in comments.
VSadov Mar 8, 2023
f844772
PR feedback on StackAllocedArguments
VSadov Mar 8, 2023
1b5f608
use the same size limit for inline arrays in the typeloader
VSadov Mar 8, 2023
bf676e4
more PR feedback
VSadov Mar 8, 2023
7d42fbc
remove SetCannotBeBlittedByObjectCloner
VSadov Mar 8, 2023
7fff2be
moving GetInlineArrayLength to MetadataType and related changes
VSadov Mar 10, 2023
2d403ee
use type.Context.Target.PointerSize
VSadov Mar 10, 2023
f6f6acc
lost resources change
VSadov Mar 10, 2023
d68cfa2
fix for x86
VSadov Mar 12, 2023
ebc698b
Do not make InlineArrayType an inline array just yet.
VSadov Mar 13, 2023
9373c77
CORINFO_FLG_INDEXABLE_FIELDS
VSadov Mar 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
Next Next commit
initial port from the prototype
  • Loading branch information
@VSadov
VSadov committed Mar 10, 2023
commit da7b00b4fb57321773e5a8f5bed1efcc46108307
2 changes: 1 addition & 1 deletion src/coreclr/inc/corinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ enum CorInfoFlag
CORINFO_FLG_ARRAY = 0x00080000, // class is an array class (initialized differently)
CORINFO_FLG_OVERLAPPING_FIELDS = 0x00100000, // struct or class has fields that overlap (aka union)
CORINFO_FLG_INTERFACE = 0x00200000, // it is an interface
CORINFO_FLG_DONT_DIG_FIELDS = 0x00400000, // don't ask field info, AOT can't rely on it (used for types outside of AOT compilation version bubble)
CORINFO_FLG_DONT_DIG_FIELDS = 0x00400000, // don't ask field info (used for types outside of AOT compilation version bubble and value arrays)
Copy link
Member Author

@VSadov VSadov Mar 4, 2023

Choose a reason for hiding this comment

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

This flag was called CORINFO_FLG_DONT_PROMOTE at the time of the prototype. We do not want inline arrays to promote as indexing relies on contiguous layout.

I assume the flag still has the effect of suppressing field promotion.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Mar 6, 2023

Choose a reason for hiding this comment

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

We should double check with the JIT team - I would think we should be setting similar flags that we set for explicit layout structs and explicit layout structs don't set this one.

Copy link
Member Author

@VSadov VSadov Mar 11, 2023

Choose a reason for hiding this comment

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

@dotnet/jit-contrib can someone from the JIT team take a look at this part?

Copy link
Member

@AndyAyersMS AndyAyersMS Mar 13, 2023

Choose a reason for hiding this comment

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

CORINFO_FLG_DONT_DIG_FIELDS doesn't always suppress promotion (eg a single gc ref field struct will still get promoted).

I would introduce a new flag describing this, something like CORINFO_FLG_INDEXABLE_FIELDS, and update the jit to block promotion when it sees it. Looks like there is still one free bit?

Down the road we might be doing more flexible partial promotion and perhaps even handle cases like these where we know exactly which fields are being touched over some span of code.

Copy link
Member Author

@VSadov VSadov Mar 13, 2023

Choose a reason for hiding this comment

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

update the jit to block promotion when it sees it

Is there an easy way to find all the places that need to care about the bit?

Copy link
Member

@jakobbotsch jakobbotsch Mar 16, 2023

Choose a reason for hiding this comment

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

@VSadov I do think we need a JIT-EE GUID update here -- otherwise it can expose all sorts of latent issues for the JIT team once we get newer collections with this flag that older JITs do not handle.

Copy link
Member Author

@VSadov VSadov Mar 16, 2023

Choose a reason for hiding this comment

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

Makes sense. Thanks!! I will change the guid in a follow up change.

In theory the presence of the attribute will imply the new runtime and prevent many versioning issues, but it is better to be sure.

Copy link
Member Author

@VSadov VSadov Mar 16, 2023

Choose a reason for hiding this comment

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

how hard would it be to begin consuming the support?

Fairly easy. Roslyn will add a layer of convenience, but otherwise the feature can be used directly.
I’ve added some use cases in corlib and libraries as a part of the change.

The only usual caveat with using a feature this early is possibly having to change things if the feature changes based on feedback.

Copy link
Member Author

@VSadov VSadov Mar 16, 2023

Choose a reason for hiding this comment

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

@jakobbotsch Sorry, did not notice your yesterdays comment.

Copy link
Member

@jakobbotsch jakobbotsch Mar 17, 2023

Choose a reason for hiding this comment

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

In theory the presence of the attribute will imply the new runtime and prevent many versioning issues, but it is better to be sure.

The problem for the JIT team is that we disambiguate compatibility of SPMI collections via the JIT-EE GUID. We automatically collect every Sunday, so after that we will start seeing contexts with the new flag set and no indication that newer JITs are needed to properly handle this. That might not cause any problems except silent bad codegen for our SPMI runs, but it could also conceivably cause spurious assertion failures.

FWIW, @EgorBo just merged #83430 which includes a JIT-EE GUID bump, so you won't need to do that in a follow-up change after all.

Fairly easy. Roslyn will add a layer of convenience, but otherwise the feature can be used directly.
I’ve added some use cases in corlib and libraries as a part of the change.

I've looked at some of the examples, that looks fairly straightforward. I guess the main thing is how to get access to InlineArrayAttribute from the programs I'm generating.

CORINFO_FLG_CUSTOMLAYOUT = 0x00800000, // does this struct have custom layout?
CORINFO_FLG_CONTAINS_GC_PTR = 0x01000000, // does the class contain a gc ptr ?
CORINFO_FLG_DELEGATE = 0x02000000, // is this a subclass of delegate or multicast delegate ?
Expand Down
12 changes: 12 additions & 0 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2043,6 +2043,18 @@ private uint getClassAttribsInternal(TypeDesc type)
result |= CorInfoFlag.CORINFO_FLG_ABSTRACT;
}

// TODO: VS disable struct promotion for valArr
//if (type is InstantiatedType it)
//{
// if (it.Name == "ValueArray`2" && it.Namespace == "System")
// {
// if (it.Instantiation[1] is ArrayType arr && arr.Rank > 1)
// {
// result |= CorInfoFlag.CORINFO_FLG_DONT_DIG_FIELDS;
// }
// }
//}

#if READYTORUN
if (!_compilation.CompilationModuleGroup.VersionsWithType(type))
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ public enum CorInfoFlag : uint
CORINFO_FLG_ARRAY = 0x00080000, // class is an array class (initialized differently)
CORINFO_FLG_OVERLAPPING_FIELDS = 0x00100000, // struct or class has fields that overlap (aka union)
CORINFO_FLG_INTERFACE = 0x00200000, // it is an interface
CORINFO_FLG_DONT_DIG_FIELDS = 0x00400000, // don't try to ask about fields outside of AOT compilation version bubble
CORINFO_FLG_DONT_DIG_FIELDS = 0x00400000, // don't try to ask about fields outside of AOT compilation version bubble or value arrays
CORINFO_FLG_CUSTOMLAYOUT = 0x00800000, // does this struct have custom layout?
CORINFO_FLG_CONTAINS_GC_PTR = 0x01000000, // does the class contain a gc ptr ?
CORINFO_FLG_DELEGATE = 0x02000000, // is this a subclass of delegate or multicast delegate ?
Expand Down
45 changes: 45 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,28 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty
layoutMetadata.Size,
out instanceByteSizeAndAlignment);

// TODO: VS adjust size
//if (type is InstantiatedType it)
//{
// if (it.Name == "ValueArray`2" && it.Namespace == "System")
// {
// if (it.Instantiation[1] is ArrayType arr)
// {
// int repeat = arr.Rank;

// if (!instanceSizeAndAlignment.Size.IsIndeterminate)
// {
// instanceSizeAndAlignment.Size = new LayoutInt(instanceSizeAndAlignment.Size.AsInt * repeat);
// }

// if (!instanceByteSizeAndAlignment.Size.IsIndeterminate)
// {
// instanceByteSizeAndAlignment.Size = new LayoutInt(instanceByteSizeAndAlignment.Size.AsInt * repeat);
// }
// }
// }
//}

ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout
{
IsAutoLayoutOrHasAutoLayoutFields = hasAutoLayoutField,
Expand Down Expand Up @@ -746,6 +768,29 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
classLayoutSize: 0,
byteCount: out instanceByteSizeAndAlignment);

// TODO: VS adjust size
//if (type is InstantiatedType it)
//{
// if (it.Name == "ValueArray`2" && it.Namespace == "System")
// {
// if (it.Instantiation[1] is ArrayType arr)
// {
// int repeat = arr.Rank;

// if (!instanceSizeAndAlignment.Size.IsIndeterminate)
// {
// instanceSizeAndAlignment.Size = new LayoutInt(instanceSizeAndAlignment.Size.AsInt * repeat);
// }

// if (!instanceByteSizeAndAlignment.Size.IsIndeterminate)
// {
// instanceByteSizeAndAlignment.Size = new LayoutInt(instanceByteSizeAndAlignment.Size.AsInt * repeat);
// }
// }
// }
//}


ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout
{
IsAutoLayoutOrHasAutoLayoutFields = true,
Expand Down
44 changes: 34 additions & 10 deletions src/coreclr/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,36 @@ public static GCPointerMap FromInstanceLayout(DefType type)
{
Debug.Assert(type.ContainsGCPointers);

GCPointerMapBuilder builder = new GCPointerMapBuilder(type.InstanceByteCount.AsInt, type.Context.Target.PointerSize);
FromInstanceLayoutHelper(ref builder, type);
int pointerSize = type.Context.Target.PointerSize;
GCPointerMapBuilder builder = new GCPointerMapBuilder(type.InstanceByteCount.AsInt, pointerSize);
FromInstanceLayoutHelper(ref builder, type, pointerSize);

return builder.ToGCMap();
}

private static void FromInstanceLayoutHelper(ref GCPointerMapBuilder builder, DefType type)
private static void FromInstanceLayoutHelper(ref GCPointerMapBuilder builder, DefType type, int pointerSize)
{
if (!type.IsValueType && type.HasBaseType)
{
DefType baseType = type.BaseType;
GCPointerMapBuilder baseLayoutBuilder = builder.GetInnerBuilder(0, baseType.InstanceByteCount.AsInt);
FromInstanceLayoutHelper(ref baseLayoutBuilder, baseType);
FromInstanceLayoutHelper(ref baseLayoutBuilder, baseType, pointerSize);
}

int repeat = 1;

//TODO: VS compute repeat
//if (type is InstantiatedType it)
//{
// if (it.Name == "ValueArray`2" && it.Namespace == "System")
// {
// if (it.Instantiation[1] is ArrayType arr)
// {
// repeat = arr.Rank;
// }
// }
//}

foreach (FieldDesc field in type.GetFields())
{
if (field.IsStatic)
Expand All @@ -37,16 +52,23 @@ private static void FromInstanceLayoutHelper(ref GCPointerMapBuilder builder, De
TypeDesc fieldType = field.FieldType;
if (fieldType.IsGCPointer)
{
builder.MarkGCPointer(field.Offset.AsInt);
for (int i = 0; i < repeat; i++)
{
builder.MarkGCPointer(field.Offset.AsInt + pointerSize * i);
}
}
else if (fieldType.IsValueType)
{
var fieldDefType = (DefType)fieldType;
if (fieldDefType.ContainsGCPointers)
{
GCPointerMapBuilder innerBuilder =
builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt);
FromInstanceLayoutHelper(ref innerBuilder, fieldDefType);
for (int i = 0; i < repeat; i++)
{
int fieldSize = fieldDefType.InstanceByteCount.AsInt;
GCPointerMapBuilder innerBuilder =
builder.GetInnerBuilder(field.Offset.AsInt + fieldSize * i, fieldSize);
FromInstanceLayoutHelper(ref innerBuilder, fieldDefType, pointerSize);
}
}
}
}
Expand Down Expand Up @@ -76,9 +98,10 @@ public static GCPointerMap FromStaticLayout(DefType type)
var fieldDefType = (DefType)fieldType;
if (fieldDefType.ContainsGCPointers)
{
int pointerSize = type.Context.Target.PointerSize;
GCPointerMapBuilder innerBuilder =
builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt);
FromInstanceLayoutHelper(ref innerBuilder, fieldDefType);
FromInstanceLayoutHelper(ref innerBuilder, fieldDefType, pointerSize);
Copy link
Member

@MichalStrehovsky MichalStrehovsky Mar 6, 2023

Choose a reason for hiding this comment

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

pointerSize can be easily obtained from fieldDefType.Context.Target.PointerSize - we don't need to pass it around.

Copy link
Member Author

@VSadov VSadov Mar 7, 2023

Choose a reason for hiding this comment

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

Thanks!

At the time this piece was written (in the prototype) I was not sure there is a good/cheap way to get the pointer size once inside the layout algorithm.

}
}
}
Expand Down Expand Up @@ -109,9 +132,10 @@ public static GCPointerMap FromThreadStaticLayout(DefType type)
var fieldDefType = (DefType)fieldType;
if (fieldDefType.ContainsGCPointers)
{
int pointerSize = type.Context.Target.PointerSize;
GCPointerMapBuilder innerBuilder =
builder.GetInnerBuilder(field.Offset.AsInt, fieldDefType.InstanceByteCount.AsInt);
FromInstanceLayoutHelper(ref innerBuilder, fieldDefType);
FromInstanceLayoutHelper(ref innerBuilder, fieldDefType, pointerSize);
}
}
}
Expand Down
12 changes: 11 additions & 1 deletion src/coreclr/vm/class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1384,6 +1384,16 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW!
LIMITED_METHOD_CONTRACT;
m_VMFlags |= (DWORD)VMFLAG_HAS_FIELDS_WHICH_MUST_BE_INITED;
}
BOOL HasValueArrayFlagSet()
{
LIMITED_METHOD_CONTRACT;
return (m_VMFlags & VMFLAG_VALUE_ARRAY);
}
void SetValueArrayFlag()
{
LIMITED_METHOD_CONTRACT;
m_VMFlags |= (DWORD)VMFLAG_VALUE_ARRAY;
}
void SetCannotBeBlittedByObjectCloner()
{
/* no op */
Expand Down Expand Up @@ -1723,7 +1733,7 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW!
VMFLAG_BESTFITMAPPING = 0x00004000, // BestFitMappingAttribute.Value
VMFLAG_THROWONUNMAPPABLECHAR = 0x00008000, // BestFitMappingAttribute.ThrowOnUnmappableChar

// unused = 0x00010000,
VMFLAG_VALUE_ARRAY = 0x00010000,
VMFLAG_NO_GUID = 0x00020000,
VMFLAG_HASNONPUBLICFIELDS = 0x00040000,
VMFLAG_HAS_CUSTOM_FIELD_ALIGNMENT = 0x00080000,
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3552,6 +3552,10 @@ uint32_t CEEInfo::getClassAttribsInternal (CORINFO_CLASS_HANDLE clsHnd)
}
if (pClass->HasExplicitFieldOffsetLayout() && pClass->HasOverlaidField())
ret |= CORINFO_FLG_OVERLAPPING_FIELDS;

if (pClass->HasValueArrayFlagSet())
ret |= CORINFO_FLG_DONT_DIG_FIELDS;

if (VMClsHnd.IsCanonicalSubtype())
ret |= CORINFO_FLG_SHAREDINST;

Expand Down
96 changes: 72 additions & 24 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,25 @@ MethodTableBuilder::BuildMethodTableThrowing(
&pByValueClassCache, bmtMFDescs, bmtFP,
&totalDeclaredFieldSize);

// TODO: VS detect valArr
//if (!bmtGenericsInfo->fContainsGenericVariables &&
// g_pValueArrayClass != NULL &&
// g_pValueArrayClass->GetCl() == GetCl() &&
// g_pValueArrayClass->GetModule() == bmtInternal->pType->GetModule())
//{
// TypeHandle lengthMarker = bmtGenericsInfo->GetInstantiation()[1];
// if (lengthMarker.IsArray())
// {
// DWORD rank = lengthMarker.GetRank();
// if (rank > 1)
// {
// bmtFP->NumValueArrayElements = rank;
// // there is no scenario when tearing a value array into pieces is desirable.
// GetHalfBakedClass()->SetValueArrayFlag();
// }
// }
//}

// Place regular static fields
PlaceRegularStaticFields();

Expand All @@ -1713,6 +1732,11 @@ MethodTableBuilder::BuildMethodTableThrowing(

_ASSERTE(HasLayout());

if (bmtFP->NumValueArrayElements)
{
GetLayoutInfo()->m_cbManagedSize *= bmtFP->NumValueArrayElements;
}

bmtFP->NumInstanceFieldBytes = GetLayoutInfo()->m_cbManagedSize;

// For simple Blittable types we still need to check if they have any overlapping
Expand Down Expand Up @@ -8318,6 +8342,16 @@ VOID MethodTableBuilder::PlaceInstanceFields(MethodTable ** pByValueClassCach
BuildMethodTableThrowException(IDS_CLASSLOAD_FIELDTOOLARGE);
}

if (bmtFP->NumValueArrayElements > 0)
{
dwNumInstanceFieldBytes *= bmtFP->NumValueArrayElements;

if (pFieldDescList[0].IsByValue())
{
dwNumGCPointerSeries *= bmtFP->NumValueArrayElements;
}
}

bmtFP->NumInstanceFieldBytes = dwNumInstanceFieldBytes;

bmtFP->NumGCPointerSeries = dwNumGCPointerSeries;
Expand Down Expand Up @@ -11494,12 +11528,19 @@ VOID MethodTableBuilder::HandleGCForValueClasses(MethodTable ** pByValueClassCac

}

DWORD repeat = 1;
if (bmtFP->NumValueArrayElements > 0)
{
_ASSERTE(bmtEnumFields->dwNumInstanceFields == 1);
repeat = bmtFP->NumValueArrayElements;
}

// Build the pointer series map for this pointers in this instance
pSeries = ((CGCDesc*)pMT)->GetLowestSeries();
if (bmtFP->NumInstanceGCPointerFields)
{
// See gcdesc.h for an explanation of why we adjust by subtracting BaseSize
pSeries->SetSeriesSize( (size_t) (bmtFP->NumInstanceGCPointerFields * TARGET_POINTER_SIZE) - (size_t) pMT->GetBaseSize());
pSeries->SetSeriesSize((size_t)(bmtFP->NumInstanceGCPointerFields * repeat * TARGET_POINTER_SIZE) - (size_t)pMT->GetBaseSize());
pSeries->SetSeriesOffset(bmtFP->GCPointerFieldStart + OBJECT_SIZE);
pSeries++;
}
Expand All @@ -11509,44 +11550,51 @@ VOID MethodTableBuilder::HandleGCForValueClasses(MethodTable ** pByValueClassCac
{
if (pFieldDescList[i].IsByValue())
{
MethodTable *pByValueMT = pByValueClassCache[i];
MethodTable* pByValueMT = pByValueClassCache[i];

if (pByValueMT->ContainsPointers())
{
// Offset of the by value class in the class we are building, does NOT include Object
DWORD dwCurrentOffset = pFieldDescList[i].GetOffset_NoLogging();
DWORD dwElementSize = pByValueMT->GetBaseSize() - OBJECT_BASESIZE;

// The by value class may have more than one pointer series
CGCDescSeries * pByValueSeries = CGCDesc::GetCGCDescFromMT(pByValueMT)->GetLowestSeries();
SIZE_T dwNumByValueSeries = CGCDesc::GetCGCDescFromMT(pByValueMT)->GetNumSeries();

for (SIZE_T j = 0; j < dwNumByValueSeries; j++)
for (DWORD r = 0; r < repeat; r++)
Copy link
Member

@jkotas jkotas Mar 4, 2023

Choose a reason for hiding this comment

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

If the value is fully covered by one series, we should only emit one series for the repeated value.

Copy link
Member Author

@VSadov VSadov Mar 4, 2023

Choose a reason for hiding this comment

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

Like in arrays? I think that pattern requires that the element count is stored in the instance.

Copy link
Member Author

@VSadov VSadov Mar 4, 2023

Choose a reason for hiding this comment

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

Except for side-by-side objects. That we can express as one combined sequence.

Copy link
Member Author

@VSadov VSadov Mar 4, 2023

Choose a reason for hiding this comment

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

That seems like can be handled in more general terms - including regular structs.
If it is an all-objects instance (recursively) and it is not an explicit layout, we can put in just one serie for the entire size of the instance.

Copy link
Member Author

@VSadov VSadov Mar 7, 2023

Choose a reason for hiding this comment

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

I've looked at this from implementation point. Folding GC series for side-by-side objects from separate struct fields is certainly doable, but a bit involved, especially on the CoreClr side.

I think it is better if this is handled as a separate PR as it is an optimization and we may need to iterate a bit on it. There is also some risk there will be bugs or regressions.

Copy link
Member Author

@VSadov VSadov Mar 7, 2023

Choose a reason for hiding this comment

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

Logged an issue for this - #83111

{
size_t cbSeriesSize;
size_t cbSeriesOffset;
// The by value class may have more than one pointer series
CGCDescSeries* pByValueSeries = CGCDesc::GetCGCDescFromMT(pByValueMT)->GetLowestSeries();
SIZE_T dwNumByValueSeries = CGCDesc::GetCGCDescFromMT(pByValueMT)->GetNumSeries();

_ASSERTE(pSeries <= CGCDesc::GetCGCDescFromMT(pMT)->GetHighestSeries());
for (SIZE_T j = 0; j < dwNumByValueSeries; j++)
{
size_t cbSeriesSize;
size_t cbSeriesOffset;

_ASSERTE(pSeries <= CGCDesc::GetCGCDescFromMT(pMT)->GetHighestSeries());

cbSeriesSize = pByValueSeries->GetSeriesSize();
cbSeriesSize = pByValueSeries->GetSeriesSize();

// Add back the base size of the by value class, since it's being transplanted to this class
cbSeriesSize += pByValueMT->GetBaseSize();
// Add back the base size of the by value class, since it's being transplanted to this class
cbSeriesSize += pByValueMT->GetBaseSize();

// Subtract the base size of the class we're building
cbSeriesSize -= pMT->GetBaseSize();
// Subtract the base size of the class we're building
cbSeriesSize -= pMT->GetBaseSize();

// Set current series we're building
pSeries->SetSeriesSize(cbSeriesSize);
// Set current series we're building
pSeries->SetSeriesSize(cbSeriesSize);

// Get offset into the value class of the first pointer field (includes a +Object)
cbSeriesOffset = pByValueSeries->GetSeriesOffset();
// Get offset into the value class of the first pointer field (includes a +Object)
cbSeriesOffset = pByValueSeries->GetSeriesOffset();

// Add it to the offset of the by value class in our class
cbSeriesOffset += dwCurrentOffset;
// Add element N offset
cbSeriesOffset += r * dwElementSize;

pSeries->SetSeriesOffset(cbSeriesOffset); // Offset of field
pSeries++;
pByValueSeries++;
// Add it to the offset of the by value class in our class
cbSeriesOffset += dwCurrentOffset;

pSeries->SetSeriesOffset(cbSeriesOffset); // Offset of field
pSeries++;
pByValueSeries++;
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/methodtablebuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2011,6 +2011,7 @@ class MethodTableBuilder
DWORD NumInstanceGCPointerFields; // does not include inherited pointer fields
DWORD NumGCPointerSeries;
DWORD NumInstanceFieldBytes;
DWORD NumValueArrayElements;

bool fIsByRefLikeType;
bool fHasFixedAddressValueTypes;
Expand Down