Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Optimization: Reduce many array allocations in GetCustomAtttributes()'s inherited path #20779

Merged
merged 8 commits into from
Nov 4, 2018
Merged

Optimization: Reduce many array allocations in GetCustomAtttributes()'s inherited path #20779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
622b74e
Optimization: avoid 2 array allocations in inherited attribute misses
NickCraver Nov 3, 2018
cb2305f
Move RuntimeType empty array cache generation to Attribute.CreateAttr…
NickCraver Nov 3, 2018
1b459b9
CustomAttributes: remove needless array copy in the inherited hit case
NickCraver Nov 3, 2018
d6004ae
GetCusomAttributes: use ListBuilder<object> for inheritance crawls
NickCraver Nov 3, 2018
496a760
Attribute caching: use Array.CreateInstance() directly on RuntimeType
NickCraver Nov 3, 2018
70dc679
Ref passing for RuntimeType.ListBuilder<object> & CustomAttribute sim…
NickCraver Nov 3, 2018
f6307e3
Merge upstream master
NickCraver Nov 3, 2018
b2ac5c6
Tidy up RuntimeType casts from object
NickCraver Nov 3, 2018
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
61 changes: 37 additions & 24 deletions src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ internal static IList<CustomAttributeData> GetCustomAttributesInternal(RuntimeTy

IList<CustomAttributeData> cad = GetCustomAttributes(target.GetRuntimeModule(), target.MetadataToken);

Attribute[] a = PseudoCustomAttribute.GetCustomAttributes((RuntimeType)target, typeof(object) as RuntimeType, out int pcaCount);
Attribute[] a = PseudoCustomAttribute.GetCustomAttributes((RuntimeType)target, (RuntimeType)typeof(object), out int pcaCount);

if (pcaCount == 0)
return cad;
Expand All @@ -83,7 +83,7 @@ internal static IList<CustomAttributeData> GetCustomAttributesInternal(RuntimeFi

IList<CustomAttributeData> cad = GetCustomAttributes(target.GetRuntimeModule(), target.MetadataToken);

Attribute[] a = PseudoCustomAttribute.GetCustomAttributes((RuntimeFieldInfo)target, typeof(object) as RuntimeType, out int pcaCount);
Attribute[] a = PseudoCustomAttribute.GetCustomAttributes((RuntimeFieldInfo)target, (RuntimeType)typeof(object), out int pcaCount);

if (pcaCount == 0)
return cad;
Expand All @@ -104,7 +104,7 @@ internal static IList<CustomAttributeData> GetCustomAttributesInternal(RuntimeMe

IList<CustomAttributeData> cad = GetCustomAttributes(target.GetRuntimeModule(), target.MetadataToken);

Attribute[] a = PseudoCustomAttribute.GetCustomAttributes((RuntimeMethodInfo)target, typeof(object) as RuntimeType, out int pcaCount);
Attribute[] a = PseudoCustomAttribute.GetCustomAttributes((RuntimeMethodInfo)target, (RuntimeType)typeof(object), out int pcaCount);

if (pcaCount == 0)
return cad;
Expand Down Expand Up @@ -156,7 +156,7 @@ internal static IList<CustomAttributeData> GetCustomAttributesInternal(RuntimeAs

IList<CustomAttributeData> cad = GetCustomAttributes((RuntimeModule)target.ManifestModule, RuntimeAssembly.GetToken(target.GetNativeHandle()));

Attribute[] a = PseudoCustomAttribute.GetCustomAttributes(target, typeof(object) as RuntimeType, out int pcaCount);
Attribute[] a = PseudoCustomAttribute.GetCustomAttributes(target, (RuntimeType)typeof(object), out int pcaCount);

if (pcaCount == 0)
return cad;
Expand All @@ -177,7 +177,7 @@ internal static IList<CustomAttributeData> GetCustomAttributesInternal(RuntimePa

IList<CustomAttributeData> cad = GetCustomAttributes(target.GetRuntimeModule(), target.MetadataToken);

Attribute[] a = PseudoCustomAttribute.GetCustomAttributes(target, typeof(object) as RuntimeType, out int pcaCount);
Attribute[] a = PseudoCustomAttribute.GetCustomAttributes(target, (RuntimeType)typeof(object), out int pcaCount);

if (pcaCount == 0)
return cad;
Expand Down Expand Up @@ -1276,17 +1276,17 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp
return attributes;
}

List<object> result = new List<object>();
RuntimeType.ListBuilder<object> result = new RuntimeType.ListBuilder<object>();
bool mustBeInheritable = false;
bool useObjectArray = (caType == null || caType.IsValueType || caType.ContainsGenericParameters);
Type arrayType = useObjectArray ? typeof(object) : caType;
RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : caType;

while (pcaCount > 0)
result.Add(pca[--pcaCount]);

while (type != (RuntimeType)typeof(object) && type != null)
{
object[] attributes = GetCustomAttributes(type.GetRuntimeModule(), type.MetadataToken, 0, caType, mustBeInheritable, result);
object[] attributes = GetCustomAttributes(type.GetRuntimeModule(), type.MetadataToken, 0, caType, mustBeInheritable, ref result);
mustBeInheritable = true;
for (int i = 0; i < attributes.Length; i++)
result.Add(attributes[i]);
Expand All @@ -1295,7 +1295,10 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp
}
Copy link
Member

@jkotas jkotas Nov 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You can replace List<object> with ListBuilder<object> to save another allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I follow this note - ListBuilder<T> is private inside RuntimeType and expects a known capacity at start, do you want to consider changing either of those things? Or aiming at ValueListBuilder<T>?

Copy link
Member

Choose a reason for hiding this comment

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

Default ListBuilder<T> constructor does not need capacity, and changing it to internal should be fine too.

ValueListBuilder would work too, but probably not as well as ListBuilder<T> in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it - I'm still figuring out what scope of changes is acceptable in which areas of CoreFX/CoreCLR, so I really appreciate the feedback! ListBuilder<T> is exposed/used now, and I've greatly expanded the benchmarks above to illustrate the gains there. Thanks!


object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count);
Array.Copy(result.ToArray(), 0, typedResult, 0, result.Count);
for (var i = 0; i < result.Count; i++)
{
typedResult[i] = result[i];
}
return typedResult;
}

Expand All @@ -1319,17 +1322,17 @@ internal static object[] GetCustomAttributes(RuntimeMethodInfo method, RuntimeTy
return attributes;
}

List<object> result = new List<object>();
RuntimeType.ListBuilder<object> result = new RuntimeType.ListBuilder<object>();
bool mustBeInheritable = false;
bool useObjectArray = (caType == null || caType.IsValueType || caType.ContainsGenericParameters);
Type arrayType = useObjectArray ? typeof(object) : caType;
RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : caType;

while (pcaCount > 0)
result.Add(pca[--pcaCount]);

while (method != null)
{
object[] attributes = GetCustomAttributes(method.GetRuntimeModule(), method.MetadataToken, 0, caType, mustBeInheritable, result);
object[] attributes = GetCustomAttributes(method.GetRuntimeModule(), method.MetadataToken, 0, caType, mustBeInheritable, ref result);
mustBeInheritable = true;
for (int i = 0; i < attributes.Length; i++)
result.Add(attributes[i]);
Expand All @@ -1338,7 +1341,10 @@ internal static object[] GetCustomAttributes(RuntimeMethodInfo method, RuntimeTy
}

object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count);
Array.Copy(result.ToArray(), 0, typedResult, 0, result.Count);
for (var i = 0; i < result.Count; i++)
{
typedResult[i] = result[i];
}
return typedResult;
}

Expand Down Expand Up @@ -1445,6 +1451,7 @@ private static bool IsCustomAttributeDefined(
RuntimeType attributeType;
IRuntimeMethodInfo ctor;
bool ctorHasParameters, isVarArg;
RuntimeType.ListBuilder<object> derivedAttributes = default;

// Optimization for the case where attributes decorate entities in the same assembly in which case
// we can cache the successful APTCA check between the decorated and the declared assembly.
Expand All @@ -1455,7 +1462,7 @@ private static bool IsCustomAttributeDefined(
CustomAttributeRecord caRecord = car[i];

if (FilterCustomAttributeRecord(caRecord, scope, ref lastAptcaOkAssembly,
decoratedModule, decoratedMetadataToken, attributeFilterType, mustBeInheritable, null, null,
decoratedModule, decoratedMetadataToken, attributeFilterType, mustBeInheritable, null, ref derivedAttributes,
out attributeType, out ctor, out ctorHasParameters, out isVarArg))
return true;
}
Expand All @@ -1480,18 +1487,19 @@ private static bool IsCustomAttributeDefined(
private static unsafe object[] GetCustomAttributes(
RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount, RuntimeType attributeFilterType)
{
return GetCustomAttributes(decoratedModule, decoratedMetadataToken, pcaCount, attributeFilterType, false, null);
RuntimeType.ListBuilder<object> _ = default;
return GetCustomAttributes(decoratedModule, decoratedMetadataToken, pcaCount, attributeFilterType, false, ref _);
}

private static unsafe object[] GetCustomAttributes(
RuntimeModule decoratedModule, int decoratedMetadataToken, int pcaCount,
RuntimeType attributeFilterType, bool mustBeInheritable, IList derivedAttributes)
RuntimeType attributeFilterType, bool mustBeInheritable, ref RuntimeType.ListBuilder<object> derivedAttributes)
{
MetadataImport scope = decoratedModule.MetadataImport;
CustomAttributeRecord[] car = CustomAttributeData.GetCustomAttributeRecords(decoratedModule, decoratedMetadataToken);

bool useObjectArray = (attributeFilterType == null || attributeFilterType.IsValueType || attributeFilterType.ContainsGenericParameters);
Type arrayType = useObjectArray ? typeof(object) : attributeFilterType;
RuntimeType arrayType = useObjectArray ? (RuntimeType)typeof(object) : attributeFilterType;

if (attributeFilterType == null && car.Length == 0)
return CreateAttributeArrayHelper(arrayType, 0);
Expand Down Expand Up @@ -1519,7 +1527,7 @@ private static unsafe object[] GetCustomAttributes(

if (!FilterCustomAttributeRecord(caRecord, scope, ref lastAptcaOkAssembly,
decoratedModule, decoratedMetadataToken, attributeFilterType, mustBeInheritable,
attributes, derivedAttributes,
attributes, ref derivedAttributes,
out attributeType, out ctor, out ctorHasParameters, out isVarArg))
continue;

Expand Down Expand Up @@ -1647,7 +1655,7 @@ private static unsafe bool FilterCustomAttributeRecord(
RuntimeType attributeFilterType,
bool mustBeInheritable,
object[] attributes,
IList derivedAttributes,
ref RuntimeType.ListBuilder<object> derivedAttributes,
out RuntimeType attributeType,
out IRuntimeMethodInfo ctor,
out bool ctorHasParameters,
Expand All @@ -1671,7 +1679,7 @@ private static unsafe bool FilterCustomAttributeRecord(

// Ensure if attribute type must be inheritable that it is inhertiable
// Ensure that to consider a duplicate attribute type AllowMultiple is true
if (!AttributeUsageCheck(attributeType, mustBeInheritable, attributes, derivedAttributes))
if (!AttributeUsageCheck(attributeType, mustBeInheritable, ref derivedAttributes))
return false;

// Windows Runtime attributes aren't real types - they exist to be read as metadata only, and as such
Expand Down Expand Up @@ -1743,7 +1751,7 @@ private static unsafe bool FilterCustomAttributeRecord(

#region Private Static Methods
private static bool AttributeUsageCheck(
RuntimeType attributeType, bool mustBeInheritable, object[] attributes, IList derivedAttributes)
RuntimeType attributeType, bool mustBeInheritable, ref RuntimeType.ListBuilder<object> derivedAttributes)
{
AttributeUsageAttribute attributeUsageAttribute = null;

Expand All @@ -1756,8 +1764,7 @@ private static bool AttributeUsageCheck(
}

// Legacy: AllowMultiple ignored for none inheritable attributes

if (derivedAttributes == null)
if (derivedAttributes.Count == 0)
return true;

for (int i = 0; i < derivedAttributes.Count; i++)
Expand Down Expand Up @@ -1844,8 +1851,14 @@ private static unsafe void GetPropertyOrFieldData(
blobStart = (IntPtr)pBlobStart;
}

private static object[] CreateAttributeArrayHelper(Type elementType, int elementCount)
private static object[] CreateAttributeArrayHelper(RuntimeType elementType, int elementCount)
{
// If we have 0 elements, don't allocate a new array
if (elementCount == 0)
{
return elementType.GetEmptyArray();
}

return (object[])Array.CreateInstance(elementType, elementCount);
}
#endregion
Expand Down
13 changes: 12 additions & 1 deletion src/System.Private.CoreLib/src/System/RtType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ internal enum MemberListType
}

// Helper to build lists of MemberInfos. Special cased to avoid allocations for lists of one element.
private struct ListBuilder<T> where T : class
internal struct ListBuilder<T> where T : class
{
private T[] _items;
private T _item;
Expand Down Expand Up @@ -1453,6 +1453,7 @@ internal RuntimeType ReflectedType
private static object s_methodInstantiationsLock;
private string m_defaultMemberName;
private object m_genericCache; // Generic cache for rare scenario specific data. It is used to cache Enum names and values.
private object[] _emptyArray; // Object array cache for Attribute.GetCustomAttributes() pathological no-result case.
Copy link
Member

@jkotas jkotas Nov 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe this can reuse m_genericCache to avoid adding more fields here.

#endregion

#region Constructor
Expand Down Expand Up @@ -1620,6 +1621,11 @@ internal string GetDefaultMemberName()

return m_defaultMemberName;
}

internal object[] GetEmptyArray()
{
return _emptyArray ?? (_emptyArray = (object[])Array.CreateInstance(m_runtimeType, 0));
}
#endregion

#region Caches Accessors
Expand Down Expand Up @@ -3533,6 +3539,11 @@ public override Type GetElementType()
{
return RuntimeTypeHandle.GetElementType(this);
}

internal object[] GetEmptyArray()
{
return Cache.GetEmptyArray();
}
#endregion

#region Enums
Expand Down