From e29789f28edfc6421eab8e9075261e3b10b65a0d Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 2 Aug 2021 13:26:59 -0700 Subject: [PATCH 1/4] Break allocation of explicit layout work into segments... probalby not the right approach --- .../Common/ExplicitLayoutValidator.cs | 64 +++++++++++++++++-- 1 file changed, 57 insertions(+), 7 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index 9066f54c04cbfb..6476ecf842c0d3 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -6,8 +6,10 @@ namespace Internal.TypeSystem { - public struct ExplicitLayoutValidator + public ref struct ExplicitLayoutValidator { + private const int FieldLayoutSegmentSize = 1024; + private enum FieldLayoutTag : byte { Empty, @@ -17,20 +19,43 @@ private enum FieldLayoutTag : byte private readonly int _pointerSize; - private readonly FieldLayoutTag[] _fieldLayout; + // Break FieldLayout data storage into an initial Span, which is stack allocated, and thus + // doesn't use GC memory, and an array of arrays for data beyond the first segment of field + // layout data. This allows the typical explicit layout structure to not require memory allocation + // and larger structures to avoid allocation for unused portions of the structure + // and when a large structure is in use, then the memory allocated will not be a huge buffer + // which is vulnerable to memory fragmentation problems on 32bit platforms. + // + // Ideally, for large structures this would be an interval tree of some sort, but the large structure + // code is rarely exercised, and thus I expect would be likely to contain bugs if such a structure + // were only implemented for this feature. + private readonly FieldLayoutTag[][] _fieldLayout; + private readonly Span _firstFieldLayoutSegment; private readonly MetadataType _typeBeingValidated; - private ExplicitLayoutValidator(MetadataType type, int typeSizeInBytes) + private ExplicitLayoutValidator(MetadataType type, int typeSizeInBytes, Span firstSegment) { _typeBeingValidated = type; _pointerSize = type.Context.Target.PointerSize; - _fieldLayout = new FieldLayoutTag[typeSizeInBytes]; + _firstFieldLayoutSegment = firstSegment; + if (typeSizeInBytes > firstSegment.Length) + { + _fieldLayout = new FieldLayoutTag[((typeSizeInBytes - firstSegment.Length) / FieldLayoutSegmentSize) + 1][]; + } + else + { + _fieldLayout = null; + } } public static void Validate(MetadataType type, ComputedInstanceFieldLayout layout) { - ExplicitLayoutValidator validator = new ExplicitLayoutValidator(type, layout.ByteCountUnaligned.AsInt); + int typeSizeInBytes = layout.ByteCountUnaligned.AsInt; + Span firstSegment = stackalloc FieldLayoutTag[Math.Min(typeSizeInBytes, FieldLayoutSegmentSize)]; + + ExplicitLayoutValidator validator = new ExplicitLayoutValidator(type, typeSizeInBytes, firstSegment); + foreach (FieldAndOffset fieldAndOffset in layout.Offsets) { validator.AddToFieldLayout(fieldAndOffset.Offset.AsInt, fieldAndOffset.Field.FieldType); @@ -133,16 +158,41 @@ private void SetFieldLayout(int offset, int count, FieldLayoutTag tag) } } + private int GetSegmentIndex(int offset, out int segmentInternalIndex) + { + segmentInternalIndex = offset % FieldLayoutSegmentSize; + return (offset - _firstFieldLayoutSegment.Length) / FieldLayoutSegmentSize; + } + private void SetFieldLayout(int offset, FieldLayoutTag tag) { - FieldLayoutTag existingTag = _fieldLayout[offset]; + FieldLayoutTag existingTag; + Span segment; + int segmentInternalIndex = offset; + + if (offset >= FieldLayoutSegmentSize) + { + segment = _firstFieldLayoutSegment; + } + else + { + int segmentIndex = GetSegmentIndex(offset, out segmentInternalIndex); + if (_fieldLayout[segmentIndex] == null) + { + _fieldLayout[segmentIndex] = new FieldLayoutTag[FieldLayoutSegmentSize]; + } + segment = _fieldLayout[segmentIndex]; + } + + existingTag = segment[segmentInternalIndex]; + if (existingTag != tag) { if (existingTag != FieldLayoutTag.Empty) { ThrowFieldLayoutError(offset); } - _fieldLayout[offset] = tag; + segment[segmentInternalIndex] = tag; } } From 68aed0e9b4804439c93b2444e92f429aa5ba6f14 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 2 Aug 2021 18:19:38 -0700 Subject: [PATCH 2/4] Initial Pass at interval array --- .../Common/ExplicitLayoutValidator.cs | 238 +++++++++++++----- 1 file changed, 174 insertions(+), 64 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index 6476ecf842c0d3..76c3ea3c405a69 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -2,13 +2,51 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.Diagnostics; namespace Internal.TypeSystem { - public ref struct ExplicitLayoutValidator + public struct ExplicitLayoutValidator { - private const int FieldLayoutSegmentSize = 1024; + struct FieldLayoutInterval : IComparable + { + public FieldLayoutInterval(int start, int size, FieldLayoutTag tag) + { + Start = start; + Size = size; + Tag = tag; + } + + public int Start; + public int Size; + + public int EndSentinel + { + get + { + return Start + Size; + } + set + { + Size = value - Start; + Debug.Assert(Size >= 0); + } + } + + public FieldLayoutTag Tag; + + public int CompareTo(FieldLayoutInterval other) + { + if (other.Start == Start) + return 0; + + if (Start < other.Start ) + return -1; + + return 1; + } + } private enum FieldLayoutTag : byte { @@ -19,42 +57,22 @@ private enum FieldLayoutTag : byte private readonly int _pointerSize; - // Break FieldLayout data storage into an initial Span, which is stack allocated, and thus - // doesn't use GC memory, and an array of arrays for data beyond the first segment of field - // layout data. This allows the typical explicit layout structure to not require memory allocation - // and larger structures to avoid allocation for unused portions of the structure - // and when a large structure is in use, then the memory allocated will not be a huge buffer - // which is vulnerable to memory fragmentation problems on 32bit platforms. - // - // Ideally, for large structures this would be an interval tree of some sort, but the large structure - // code is rarely exercised, and thus I expect would be likely to contain bugs if such a structure - // were only implemented for this feature. - private readonly FieldLayoutTag[][] _fieldLayout; - private readonly Span _firstFieldLayoutSegment; + // Represent field layout bits as as a series of intervals to prevent pathological bad behavior + // involving excessively large explicit layout structures. + private readonly List _fieldLayout = new List(); private readonly MetadataType _typeBeingValidated; - private ExplicitLayoutValidator(MetadataType type, int typeSizeInBytes, Span firstSegment) + private ExplicitLayoutValidator(MetadataType type, int typeSizeInBytes) { _typeBeingValidated = type; _pointerSize = type.Context.Target.PointerSize; - _firstFieldLayoutSegment = firstSegment; - if (typeSizeInBytes > firstSegment.Length) - { - _fieldLayout = new FieldLayoutTag[((typeSizeInBytes - firstSegment.Length) / FieldLayoutSegmentSize) + 1][]; - } - else - { - _fieldLayout = null; - } } public static void Validate(MetadataType type, ComputedInstanceFieldLayout layout) { int typeSizeInBytes = layout.ByteCountUnaligned.AsInt; - Span firstSegment = stackalloc FieldLayoutTag[Math.Min(typeSizeInBytes, FieldLayoutSegmentSize)]; - - ExplicitLayoutValidator validator = new ExplicitLayoutValidator(type, typeSizeInBytes, firstSegment); + ExplicitLayoutValidator validator = new ExplicitLayoutValidator(type, typeSizeInBytes); foreach (FieldAndOffset fieldAndOffset in layout.Offsets) { @@ -100,11 +118,25 @@ private void AddToFieldLayout(int offset, TypeDesc fieldType) ThrowFieldLayoutError(offset); } - bool[] fieldORefMap = new bool[fieldSize]; + List fieldORefMap = new List(); MarkORefLocations(mdType, fieldORefMap, offset: 0); - for (int index = 0; index < fieldSize; index++) + + // Merge in fieldORefMap from structure specifying not attributed intervals as NonORef + int lastGCRegionReportedEnd = 0; + + foreach (var gcRegion in fieldORefMap) + { + SetFieldLayout(offset + lastGCRegionReportedEnd, gcRegion.Start - lastGCRegionReportedEnd, FieldLayoutTag.NonORef); + Debug.Assert(gcRegion.Tag == FieldLayoutTag.ORef); + SetFieldLayout(offset + gcRegion.Start, gcRegion.Size, gcRegion.Tag); + lastGCRegionReportedEnd = gcRegion.EndSentinel; + } + + if (fieldORefMap.Count > 0) { - SetFieldLayout(offset + index, fieldORefMap[index] ? FieldLayoutTag.ORef : FieldLayoutTag.NonORef); + int trailingRegionStart = fieldORefMap[fieldORefMap.Count - 1].EndSentinel; + int trailingRegionSize = fieldSize - trailingRegionStart; + SetFieldLayout(offset + trailingRegionStart, trailingRegionSize, FieldLayoutTag.NonORef); } } } @@ -123,7 +155,7 @@ private void AddToFieldLayout(int offset, TypeDesc fieldType) } } - private void MarkORefLocations(MetadataType type, bool[] orefMap, int offset) + private void MarkORefLocations(MetadataType type, List orefMap, int offset) { // Recurse into struct fields foreach (FieldDesc field in type.GetFields()) @@ -133,10 +165,7 @@ private void MarkORefLocations(MetadataType type, bool[] orefMap, int offset) int fieldOffset = offset + field.Offset.AsInt; if (field.FieldType.IsGCPointer) { - for (int index = 0; index < _pointerSize; index++) - { - orefMap[fieldOffset + index] = true; - } + SetFieldLayout(orefMap, offset, _pointerSize, FieldLayoutTag.ORef); } else if (field.FieldType.IsValueType) { @@ -150,52 +179,133 @@ private void MarkORefLocations(MetadataType type, bool[] orefMap, int offset) } } - private void SetFieldLayout(int offset, int count, FieldLayoutTag tag) + private void SetFieldLayout(List fieldLayoutInterval, int offset, int count, FieldLayoutTag tag) { - for (int index = 0; index < count; index++) - { - SetFieldLayout(offset + index, tag); - } - } + if (count == 0) + return; - private int GetSegmentIndex(int offset, out int segmentInternalIndex) - { - segmentInternalIndex = offset % FieldLayoutSegmentSize; - return (offset - _firstFieldLayoutSegment.Length) / FieldLayoutSegmentSize; - } + var newInterval = new FieldLayoutInterval(offset, count, tag); - private void SetFieldLayout(int offset, FieldLayoutTag tag) - { - FieldLayoutTag existingTag; - Span segment; - int segmentInternalIndex = offset; - - if (offset >= FieldLayoutSegmentSize) + int binarySearchIndex = fieldLayoutInterval.BinarySearch(new FieldLayoutInterval(offset, 0, FieldLayoutTag.Empty)); + + if (binarySearchIndex >= 0) { - segment = _firstFieldLayoutSegment; + var existingInterval = fieldLayoutInterval[binarySearchIndex]; + + // Exact match found for start of interval. + if (tag != existingInterval.Tag) + { + ThrowFieldLayoutError(offset); + } + + if (existingInterval.Size >= count) + { + // Existing interval is big enough. + } + else + { + // Expand existing interval, and then check to see if that's valid. + existingInterval.Size = count; + fieldLayoutInterval[binarySearchIndex] = existingInterval; + + ValidateAndMergeIntervalWithFollowingIntervals(fieldLayoutInterval, binarySearchIndex); + } } else { - int segmentIndex = GetSegmentIndex(offset, out segmentInternalIndex); - if (_fieldLayout[segmentIndex] == null) + // No exact start match found. + + int newIntervalLocation = ~binarySearchIndex; + + // Check for previous interval overlaps cases + if ((newIntervalLocation - 1) > 0) + { + var previousInterval = fieldLayoutInterval[newIntervalLocation - 1]; + bool tagMatches = previousInterval.Tag == tag; + + if (previousInterval.EndSentinel > offset) + { + // Previous interval overlaps. + if (!tagMatches) + { + ThrowFieldLayoutError(offset); + } + } + + if (previousInterval.EndSentinel > offset || (tagMatches && previousInterval.EndSentinel == offset)) + { + // Previous interval overlaps, or exactly matches up with new interval and tag matches. Instead + // of expanding interval set, simply expand the previous interval. + previousInterval.EndSentinel = newInterval.EndSentinel; + + fieldLayoutInterval[newIntervalLocation - 1] = previousInterval; + newIntervalLocation = newIntervalLocation - 1; + } + else + { + fieldLayoutInterval.Insert(newIntervalLocation, newInterval); + } + } + else { - _fieldLayout[segmentIndex] = new FieldLayoutTag[FieldLayoutSegmentSize]; + // New interval added at start + fieldLayoutInterval.Insert(newIntervalLocation, newInterval); } - segment = _fieldLayout[segmentIndex]; - } - existingTag = segment[segmentInternalIndex]; + ValidateAndMergeIntervalWithFollowingIntervals(fieldLayoutInterval, newIntervalLocation); + } + } - if (existingTag != tag) + private void ValidateAndMergeIntervalWithFollowingIntervals(List fieldLayoutInterval, int intervalIndex) + { + while(true) { - if (existingTag != FieldLayoutTag.Empty) + if (intervalIndex + 1 == fieldLayoutInterval.Count) { - ThrowFieldLayoutError(offset); + // existing interval is last interval. Expansion always succeeds + break; + } + else + { + var nextInterval = fieldLayoutInterval[intervalIndex + 1]; + var expandedInterval = fieldLayoutInterval[intervalIndex]; + var tag = expandedInterval.Tag; + + if (nextInterval.Start > expandedInterval.EndSentinel) + { + // Next interval does not contact existing interval. Expansion succeeded + break; + } + + if ((nextInterval.Start == expandedInterval.EndSentinel) && nextInterval.Tag != tag) + { + // Next interval starts just after existing interval, but does not match tag. Expansion succeeded + break; + } + + Debug.Assert(nextInterval.Start <= expandedInterval.EndSentinel); + // Next interval overlaps with expanded interval. + + if (nextInterval.Tag != tag) + { + ThrowFieldLayoutError(nextInterval.Start); + } + + // Expand existing interval to cover region of next interval + expandedInterval.EndSentinel = nextInterval.EndSentinel; + fieldLayoutInterval[intervalIndex] = expandedInterval; + + // Remove next interval + fieldLayoutInterval.RemoveAt(intervalIndex + 1); } - segment[segmentInternalIndex] = tag; } } + private void SetFieldLayout(int offset, int count, FieldLayoutTag tag) + { + SetFieldLayout(_fieldLayout, offset, count, tag); + } + private void ThrowFieldLayoutError(int offset) { ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadExplicitLayout, _typeBeingValidated, offset.ToStringInvariant()); From de45563343db1160fbae4e2d845550ddff476699 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 2 Aug 2021 18:36:24 -0700 Subject: [PATCH 3/4] Fix off by one error --- .../Common/ExplicitLayoutValidator.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index 76c3ea3c405a69..6027589e2c335f 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -9,7 +9,14 @@ namespace Internal.TypeSystem { public struct ExplicitLayoutValidator { - struct FieldLayoutInterval : IComparable + private enum FieldLayoutTag : byte + { + Empty, + NonORef, + ORef, + } + + private struct FieldLayoutInterval : IComparable { public FieldLayoutInterval(int start, int size, FieldLayoutTag tag) { @@ -48,13 +55,6 @@ public int CompareTo(FieldLayoutInterval other) } } - private enum FieldLayoutTag : byte - { - Empty, - NonORef, - ORef, - } - private readonly int _pointerSize; // Represent field layout bits as as a series of intervals to prevent pathological bad behavior @@ -218,7 +218,7 @@ private void SetFieldLayout(List fieldLayoutInterval, int o int newIntervalLocation = ~binarySearchIndex; // Check for previous interval overlaps cases - if ((newIntervalLocation - 1) > 0) + if (newIntervalLocation > 0) { var previousInterval = fieldLayoutInterval[newIntervalLocation - 1]; bool tagMatches = previousInterval.Tag == tag; From 6e3a8ec1c14c87385c3c0114bf013a0ce168bc19 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 3 Aug 2021 13:58:58 -0700 Subject: [PATCH 4/4] Code review feedback --- .../TypeSystem/Common/ExplicitLayoutValidator.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index 6027589e2c335f..07489f82c976aa 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -45,13 +45,7 @@ public int EndSentinel public int CompareTo(FieldLayoutInterval other) { - if (other.Start == Start) - return 0; - - if (Start < other.Start ) - return -1; - - return 1; + return Start.CompareTo(other.Start); } } @@ -186,7 +180,7 @@ private void SetFieldLayout(List fieldLayoutInterval, int o var newInterval = new FieldLayoutInterval(offset, count, tag); - int binarySearchIndex = fieldLayoutInterval.BinarySearch(new FieldLayoutInterval(offset, 0, FieldLayoutTag.Empty)); + int binarySearchIndex = fieldLayoutInterval.BinarySearch(newInterval); if (binarySearchIndex >= 0) {