Cleanup LowLevelList usage in GCLayout #116316
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
| @@ -306,9 +306,11 @@ public ushort NumVTableSlots | |||
|
|
|||
| // Sentinel static to allow us to initialize _instanceLayout to something | |||
| // and then detect that InstanceGCLayout should return null | |||
| private static LowLevelList<bool> s_emptyLayout = new LowLevelList<bool>(); | |||
| #pragma warning disable CA1825 // Can't use generic Array.Empty<T> within type loader | |||
There was a problem hiding this comment.
This is not the case anymore. It is fine use Array.Empty here.
|
Test failure is #116261 |
| @@ -306,9 +306,9 @@ public ushort NumVTableSlots | |||
|
|
|||
| // Sentinel static to allow us to initialize _instanceLayout to something | |||
| // and then detect that InstanceGCLayout should return null | |||
| private static LowLevelList<bool> s_emptyLayout = new LowLevelList<bool>(); | |||
| internal static readonly bool[] s_emptyLayout = []; | |||
There was a problem hiding this comment.
You should not need to cache in a static field - it is a deoptimization to cache it.
There was a problem hiding this comment.
The empty array instance is used as a sentinel and compared in reference. If WriteGCDescToBitfield produces empty layout, it may be treated differently. Comparing to the collection literal expression looks weird here.
| @@ -473,37 +459,27 @@ public GCLayout(RuntimeTypeHandle rtth) | |||
| Debug.Assert(MethodTable != null); | |||
|
|
|||
| _bitfield = null; | |||
| _isReferenceTypeGCLayout = false; // This field is only used for the LowLevelList<bool> path | |||
| _gcdesc = MethodTable->ContainsGCPointers ? (void**)MethodTable - 1 : null; | |||
| _size = (int)MethodTable->BaseSize; | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Writes this layout to the given bitfield. | |||
There was a problem hiding this comment.
There is no "given bitfield" anymore. This comment should be updated.
Also, should the method be renamed to GetBitField? It is not necessarily "writing to" anything.
| if (IsNone) | ||
| return; | ||
| return TypeBuilderState.s_emptyLayout; |
There was a problem hiding this comment.
I think this sentinel should be kelp private to TypeBuilderState. This check can deleted here - the one place that calls this method does this only when IsNone returns false.
| @@ -542,7 +542,7 @@ private static unsafe int CreateArrayGCDesc(LowLevelList<bool> bitfield, int ran | |||
| return numSeries; | |||
| } | |||
|
|
|||
| private static unsafe int CreateGCDesc(LowLevelList<bool> bitfield, int size, bool isValueType, bool isStatic, void* gcdesc) | |||
| private static unsafe int CreateGCDesc(bool[] bitfield, int size, bool isValueType, bool isStatic, void* gcdesc) | |||
There was a problem hiding this comment.
This method looks unreachable given that InstanceGCLayout only returns non-null layout for arrays. Delete it?
There was a problem hiding this comment.
It's also called in EETypeCreator.CreateEETypeWorker, which should be reachable.
There was a problem hiding this comment.
I believe that the other call is unreachable as well. One of them computes just the size, the other one (that you have deleted here) fills in the memory that was allocated based the size returned from the first call.
There was a problem hiding this comment.
CreateEETypeWorker calls GetInstanceGCDescSize for size and CreateInstanceGCDesc for content. Both methods are called once now.
There was a problem hiding this comment.
There is only one place in GetInstanceGCDescSize that calls CreateGCDesc. My point is that that codepath is unreachable and it can be deleted, in similar way how you deleted it CreateGCDesc from CreateInstanceGCDesc.
If I am missing something and CreateGCDesc is still reachable, it would be useful to find a test that hits it.
There was a problem hiding this comment.
Yes, the code within GetInstanceGCDescSize also has a dead path. Now the control flow should be the same with CreateInstanceGCDesc.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ba-g deadletter |
Extracted from #109114
The merging and _isReferenceTypeGCLayout paths looks unused. Not sure whether I missed anything.