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

NickCraver · 2018-11-03T01:53:46Z

This is a follow-up to many allocations recognized in #20448.

A lot of methods ultimately call through CustomAttributes.GetCustomAttributes(). In the inherited search path (default for most searches above), the inheritance path of a class is traversed, resulting in an array allocation per crawled type, a copy to the overall List and then after that - in current code - an array allocation from that list and a typed array allocation it's copied to.

However, in the common miss case as simple as:

typeof(T).GetCustomAttributes(typeof(TAttr), true);

...and many other overloads, all the same path underneath...

We can avoid the last 2 arrays in the miss case. We have the List<object> to go off of. If that's a zero-entry list, we can return an Array.Empty<TAttr>(). That's effectively what this change does.

While converting the entire attribute pipeline to generics is problematic and has issue since some object[] return abstracts aren't sealed, we can at least somewhat trivially cache an array per attribute type (only one static, ultimately from Array.Empty<T> underneath) and return that for the miss case.

There are far more wins to be had here, but they require more changes.

Benchmarks are in flux at the moment (see #20763), but the following benchmark:

[MemoryDiagnoser]
public class AttributePerf
{
    private static readonly MethodInfo AttributedOverride = typeof(AttributedClass).GetMethod(nameof(AttributedClass.FooAttributed));
    private static readonly MethodInfo NonAttributedOverride = typeof(NonAttributedClass).GetMethod(nameof(NonAttributedClass.FooAttributed));
    private static readonly MethodInfo AttributedBase = typeof(BaseClass).GetMethod(nameof(BaseClass.FooAttributed));
    private static readonly MethodInfo NonAttributedBase = typeof(BaseClass).GetMethod(nameof(BaseClass.FooUnattributed));

    [Benchmark(Description = "Class: Hit (inherit)")]
    public object[] ClassHitInherit() => typeof(AttributedClass).GetCustomAttributes(typeof(MyAttribute), true);
    [Benchmark(Description = "Class: Miss (inherit)")]
    public object[] ClassMissInherit() => typeof(NonAttributedClass).GetCustomAttributes(typeof(MyAttribute), true);

    [Benchmark(Description = "Class: Hit (no inherit)")]
    public object[] ClassHit() => typeof(AttributedClass).GetCustomAttributes(typeof(MyAttribute), false);
    [Benchmark(Description = "Class: Miss (no inherit)")]
    public object[] ClassMiss() => typeof(NonAttributedClass).GetCustomAttributes(typeof(MyAttribute), false);

    [Benchmark(Description = "Method Override: Hit (inherit)")]
    public object[] MethodOverrideHitInherit() => AttributedOverride.GetCustomAttributes(typeof(MyAttribute), true);
    [Benchmark(Description = "Method Override: Miss (inherit)")]
    public object[] MethodOverrideMissInherit() => NonAttributedOverride.GetCustomAttributes(typeof(MyAttribute), true);

    [Benchmark(Description = "Method Override: Hit (no inherit)")]
    public object[] MethodOverrideHit() => AttributedOverride.GetCustomAttributes(typeof(MyAttribute), false);
    [Benchmark(Description = "Method Override: Miss (no inherit)")]
    public object[] MethodOverrideMiss() => NonAttributedOverride.GetCustomAttributes(typeof(MyAttribute), false);

    [Benchmark(Description = "Method Base: Hit (inherit)")]
    public object[] MethodBaseHitInherit() => AttributedBase.GetCustomAttributes(typeof(MyAttribute), true);
    [Benchmark(Description = "Method Base: Miss (inherit)")]
    public object[] MethodBaseMissInherit() => NonAttributedBase.GetCustomAttributes(typeof(MyAttribute), true);

    [Benchmark(Description = "Method Base: Hit (no inherit)")]
    public object[] MethodBaseHit() => AttributedBase.GetCustomAttributes(typeof(MyAttribute), false);
    [Benchmark(Description = "Method Base: Miss (no inherit)")]
    public object[] MethodBaseMiss() => NonAttributedBase.GetCustomAttributes(typeof(MyAttribute), false);

    [My("Test")]
    public class AttributedClass : BaseClass
    {
        [My("FooAttributed")]
        public override string FooAttributed() => null;
    }

    public class NonAttributedClass : BaseClass
    {
        public override string FooUnattributed() => null;
    }

    public class BaseClass
    {
        [My("Foo")]
        public virtual string FooAttributed() => null;
        public virtual string FooUnattributed() => null;
    }

    [AttributeUsage(AttributeTargets.All, AllowMultiple = false)]
    public class MyAttribute : Attribute
    {
        private readonly string _name;
        public MyAttribute(string name) => _name = name;
    }
}

Yields the following gains with this change:

BenchmarkDotNet=v0.11.2, OS=Windows 10.0.17763.55 (1809/October2018Update/Redstone5)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview-009722
  [Host]     : .NET Core 3.0.0-preview1-27101-02 (CoreCLR 4.6.27029.02, CoreFX 4.6.27018.01), 64bit RyuJIT
  Job-TLDGVD : .NET Core ? (CoreCLR 4.6.27101.0, CoreFX 4.7.18.53005), 64bit RyuJIT
Runtime=Core  Toolchain=CoreRun

Before

Method	Mean	Error	StdDev	Median	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
'Class: Hit (inherit)'	2,893.6 ns	57.856 ns	138.620 ns	2,844.8 ns	0.1793	-	-	568 B
'Class: Miss (inherit)'	1,062.9 ns	21.184 ns	42.307 ns	1,040.4 ns	0.0343	-	-	112 B
'Class: Hit (no inherit)'	1,897.2 ns	28.440 ns	26.603 ns	1,890.7 ns	0.1221	-	-	384 B
'Class: Miss (no inherit)'	376.5 ns	7.597 ns	9.878 ns	373.5 ns	0.0072	-	-	24 B
'Method Override: Hit (inherit)'	4,015.6 ns	69.482 ns	58.020 ns	4,013.4 ns	0.2747	-	-	872 B
'Method Override: Miss (inherit)'	2,470.7 ns	48.886 ns	120.835 ns	2,433.7 ns	0.1717	-	-	544 B
'Method Override: Hit (no inherit)'	1,885.3 ns	37.046 ns	45.496 ns	1,867.3 ns	0.1259	-	-	400 B
'Method Override: Miss (no inherit)'	1,882.6 ns	36.321 ns	45.935 ns	1,868.7 ns	0.1221	-	-	384 B
'Method Base: Hit (inherit)'	2,308.3 ns	25.619 ns	20.001 ns	2,315.3 ns	0.1717	-	-	544 B
'Method Base: Miss (inherit)'	719.4 ns	14.435 ns	32.582 ns	703.6 ns	0.0277	-	-	88 B
'Method Base: Hit (no inherit)'	1,825.9 ns	11.479 ns	10.738 ns	1,828.2 ns	0.1221	-	-	384 B
'Method Base: Miss (no inherit)'	384.8 ns	7.642 ns	7.506 ns	383.3 ns	0.0072	-	-	24 B

Cached Array & loop copy

Method	Mean	Error	StdDev	Median	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
'Class: Hit (inherit)'	2,657.1 ns	59.834 ns	79.877 ns	2,615.9 ns	0.1678	-	-	536 B
'Class: Miss (inherit)'	852.2 ns	5.066 ns	4.491 ns	852.0 ns	0.0277	-	-	88 B
'Class: Hit (no inherit)'	1,880.9 ns	9.299 ns	8.243 ns	1,879.0 ns	0.1221	-	-	384 B
'Class: Miss (no inherit)'	374.1 ns	5.301 ns	4.958 ns	372.4 ns	0.0067	-	-	24 B
'Method Override: Hit (inherit)'	3,902.4 ns	32.201 ns	30.121 ns	3,899.1 ns	0.2670	-	-	840 B
'Method Override: Miss (inherit)'	2,345.2 ns	17.477 ns	16.348 ns	2,348.3 ns	0.1602	-	-	512 B
'Method Override: Hit (no inherit)'	1,967.4 ns	24.553 ns	21.765 ns	1,968.3 ns	0.1259	-	-	400 B
'Method Override: Miss (no inherit)'	1,906.1 ns	6.111 ns	4.771 ns	1,905.1 ns	0.1221	-	-	384 B
'Method Base: Hit (inherit)'	2,361.2 ns	28.074 ns	24.887 ns	2,357.8 ns	0.1602	-	-	512 B
'Method Base: Miss (inherit)'	519.0 ns	3.167 ns	2.963 ns	518.6 ns	0.0200	-	-	64 B
'Method Base: Hit (no inherit)'	1,964.2 ns	39.248 ns	63.378 ns	1,930.2 ns	0.1221	-	-	384 B
'Method Base: Miss (no inherit)'	413.8 ns	8.035 ns	20.740 ns	409.5 ns	0.0072	-	-	24 B

Above with `ref` `ListBuilder<T>` usage

Method	Mean	Error	StdDev	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
'Class: Hit (inherit)'	2,262.8 ns	44.668 ns	56.490 ns	0.1297	-	-	416 B
'Class: Miss (inherit)'	571.1 ns	2.599 ns	2.304 ns	-	-	-	-
'Class: Hit (no inherit)'	1,735.9 ns	18.678 ns	17.471 ns	0.1221	-	-	384 B
'Class: Miss (no inherit)'	247.7 ns	5.057 ns	5.823 ns	-	-	-	-
'Method Override: Hit (inherit)'	3,383.7 ns	26.707 ns	24.982 ns	0.2251	-	-	720 B
'Method Override: Miss (inherit)'	2,055.4 ns	13.720 ns	12.834 ns	0.1297	-	-	416 B
'Method Override: Hit (no inherit)'	1,780.7 ns	34.609 ns	35.541 ns	0.1259	-	-	400 B
'Method Override: Miss (no inherit)'	1,745.9 ns	10.034 ns	9.385 ns	0.1221	-	-	384 B
'Method Base: Hit (inherit)'	2,053.4 ns	14.508 ns	13.571 ns	0.1297	-	-	416 B
'Method Base: Miss (inherit)'	375.3 ns	7.589 ns	9.868 ns	-	-	-	-
'Method Base: Hit (no inherit)'	1,780.4 ns	39.946 ns	54.679 ns	0.1221	-	-	384 B
'Method Base: Miss (no inherit)'	254.0 ns	1.739 ns	1.541 ns	-	-	-	-

The gains are somewhat trivial on a single call (2 arrays and 24 bytes), but this is a path called a lot at scale and hopefully we agree multiplier on these changes is huge.

cc @benaadams @stephentoub @adamsitnik @noahfalk

This is a follow-up to many allocations recognized in dotnet#20448. A lot of methods ultimately call through CustomAttributes.GetCustomAttributes(). In the inherited search path (default for most searches above), the inheritance path of a class is traversed, resulting in an array allocation per crawled type, a copy to the overall List<object> and then after that - in current code - an array allocation from that list and a typed array allocation it's copied to. However, in the common miss case as simple as: typeof(T).GetCustomAttributes(typeof(TAttr), true); ...and many other overloads, all the same path underneath... We can avoid the last 2 arrays in the miss case. We have the List<object> to go off of. If that's a zero-entry list, we can return an Array.Empty<TAttr>(). That's effectively what this change does. While converting the entire attribute pipeline to generics is problematic and has issue since some object[] return abstracts aren't sealed, we can at least somewhat trivially cache an array per attribute type (only one static, ultimately from Array.Empty<T> underneath) and return that for the miss case. There are far more wins to be had here, but they require more changes.

jkotas · 2018-11-03T03:04:48Z

src/System.Private.CoreLib/src/System/RtType.cs

+            {
+                if (_emptyArray == null)
+                {
+                    MethodInfo method = typeof(Array).GetMethod(nameof(Array.Empty));


This should not be using reflection to get to Array.Empty. Getting to Array.Empty this way is expensive and we are not really benefiting from caching done by Array.Empty here since we have our own cache.

It should just cache the empty array created by CreateAttributeArrayHelper instead.

We were worried about allocating a duplicate empty here, but I agree in the scheme of changes here that's a bit silly given we did so for every single call previously - simplified in latest:

return _emptyArray ?? (_emptyArray = Attribute.CreateAttributeArrayHelper(m_runtimeType, 0));

jkotas · 2018-11-03T03:10:06Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

+            {
+                return useObjectArray ? Array.Empty<object>() : caType.GetEmptyArray();
+            }
+
            object[] typedResult = CreateAttributeArrayHelper(arrayType, result.Count);
            Array.Copy(result.ToArray(), 0, typedResult, 0, result.Count);


Instead of doing result.ToArray() here that allocates a temporary array, it should be better to just go over the list and assign one element at a time. Another option would be to use a bulk copy but that's likely more expensive to setup here since there are a very few attributes in typical case.

Agreed on both counts - simple loop copy added here, perf is much better.

jkotas · 2018-11-03T03:10:59Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

@@ -1294,6 +1294,11 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp
                type = type.BaseType as RuntimeType;
            }


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

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>?

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.

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!

jkotas · 2018-11-03T03:15:37Z

src/System.Private.CoreLib/src/System/RtType.cs

@@ -1462,6 +1462,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.


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

…ibuteArrayHelper This exposes Attribute.CreateAttributeArrayHelper to internal and uses it directly on the RuntimeType caching for empty arrays. Though this allocated 1 additional array overall, it's simpler, faster to init, and still is an infinite win over the per-call allocations before this overall changesets.

This removes a .ToArray() for the sake of Array.Copy() where a simple for loop suffices and removes the allocation. Reversing the "empty" result checks is also just a bit cleaner here. This also expands the same fix to the MemberInfo path. Note: should DRY these up too (longstanding issue) - but let's do that in a separate commit for clarity.

This exposes RuntimeType.ListBuilder<T> for internal usage and replaces the List<T> allocation in GetCustomAttributes() paths to reduce allocations and increase performance in the inherited crawl paths (which is the default for many optional-parameter methods in layers above). Note: there is a subtle behavior depending on previous-null (not possible with a struct now) in AttributeUsageCheck() that I believe still behaves correctly, but could use another set of eyes and a full test suite run to confirm. object[] attributes was removed there simply because it wasn't used before - only cleaning up.

jkotas · 2018-11-03T16:01:34Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

@@ -1743,7 +1759,7 @@ internal static bool IsAttributeDefined(RuntimeModule decoratedModule, int decor

        #region Private Static Methods
        private static bool AttributeUsageCheck(
-            RuntimeType attributeType, bool mustBeInheritable, object[] attributes, IList derivedAttributes)
+            RuntimeType attributeType, bool mustBeInheritable, RuntimeType.ListBuilder<object> derivedAttributes)


ListBuilder is struct. It should be passed around via ref.

Agreed - looking at this locally. private static unsafe object[] GetCustomAttributes() on 1496 needs refactor love though, as the old method is a bit brittle there - seeing about a refactor to make this cleaner and efficient.

jkotas · 2018-11-03T16:20:39Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

+                }
+                return typedResult;
+            }
+            // Return a cached, empty array


There are other places where CreateAttributeArrayHelper is called for zero count. It would be better to have the special casing for zero length to be in CreateAttributeArrayHelper instead at every callsite.

CreateAttributeArrayHelper(Type elementType, int elementCount) { return (elementCount != 0) ? Array.CreateInstance(elementType, elementCount) : elementType.GetEmptyArray(); }

And call Array.CreateInstance in GetEmptyArray directly, without going back to CreateAttributeArrayHelper.

Also, the return type of CreateAttributeArrayHelper should be object[] to make this work well. Changing it will also fix a corner case bug with the useObjectArray special casing. object[] cannot be cast to Attribute[]. Looks like we do not have a test that covers it since you can create non-standard attributes that are ValueTypes in IL only.

Ah that explains the value types ternaries in here - I was wondering how the hell value types were allowed - had the same thought, but Attribute[] return prevented it before from being generically usable. Agree on centralizing - will push in a bit.

Note: I had to look at this twice - may be confusing to look at in text here: there are 2 instances of this helper. The one in CustomAttribute is already object[], the one on Attribute is Attribute[] (and needs to be, since the return there is more specific all the way up).

This also reverts the CreateAttributeArrayHelper => internal change, since it's no longer needed.

…plification This fixes the struct passing duplication and tweaks how we're creating arrays a bit, centralizing the zero-element checks to cover all cases as well as simplify the per-method code to rely on the fact this is happening underneath.

NickCraver · 2018-11-03T17:58:00Z

@jkotas @stephentoub another round of fixes & tweaks in - numbers updated in the post, looking good but more is still on the table. I yanked in your latest changes here and cleaned up the cases a bit per-method - ready for a look when you have time. I'll poke a bit more at the remaining allocations here - there's still a lot of arrays to relay private methods results up and down but they're shared so much I'm not seeing a clear refactor path here yet (and such should probably be a different PR, unless you disagree).

jkotas

LGTM. Thank you!

stephentoub · 2018-11-03T20:51:31Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

            bool mustBeInheritable = false;
            bool useObjectArray = (caType == null || caType.IsValueType || caType.ContainsGenericParameters);
-            Type arrayType = useObjectArray ? typeof(object) : caType;
+            RuntimeType arrayType = useObjectArray ? typeof(object) as RuntimeType : caType;


I generally think of as being used when we think it's possible the cast may fail, and want a null instead of an exception. But this should never fail, so can we use (RuntimeType)typeof(object) instead? It's also more consistent with what's used elsewhere in this method already.

Absolutely - done!

stephentoub · 2018-11-03T20:52:40Z

src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

            bool mustBeInheritable = false;
            bool useObjectArray = (caType == null || caType.IsValueType || caType.ContainsGenericParameters);
-            Type arrayType = useObjectArray ? typeof(object) : caType;
+            RuntimeType arrayType = useObjectArray ? typeof(object) as RuntimeType : caType;


Ditto... same in other similar places.

NickCraver · 2018-11-03T21:04:06Z

Just FYI: I'm perplexed on how to eliminate the overall type-as-parameter and that being the return type problem here, but I'm poking PseudoCustomAttribute now. I think it can safely use ListBuilder<T> (we should probably move this...) underneath - will throw up another PR after this one lands if so.

NickCraver · 2018-11-04T03:06:58Z

I have a few more changes to further reduce allocations - since this is reviewed I'll fire up another PR after unless it's wanted here.

Those two further reduce allocations on hits, more so in the inherited crawl path:

Method	Mean	Error	StdDev	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
'Class: Hit (inherit)'	2,402.0 ns	25.016 ns	23.400 ns	0.1221	-	-	384 B
'Class: Miss (inherit)'	578.8 ns	12.380 ns	10.338 ns	-	-	-	-
'Class: Hit (no inherit)'	2,147.8 ns	41.878 ns	58.708 ns	0.1221	-	-	384 B
'Class: Miss (no inherit)'	336.3 ns	15.546 ns	13.781 ns	-	-	-	-
'Method Override: Hit (inherit)'	3,509.8 ns	37.309 ns	34.899 ns	0.2060	-	-	656 B
'Method Override: Miss (inherit)'	2,253.3 ns	68.342 ns	78.703 ns	0.1221	-	-	384 B
'Method Override: Hit (no inherit)'	2,142.0 ns	19.804 ns	18.525 ns	0.1259	-	-	400 B
'Method Override: Miss (no inherit)'	2,151.8 ns	20.700 ns	19.362 ns	0.1221	-	-	384 B
'Method Base: Hit (inherit)'	2,260.4 ns	30.418 ns	25.401 ns	0.1221	-	-	384 B
'Method Base: Miss (inherit)'	367.3 ns	3.426 ns	3.037 ns	-	-	-	-
'Method Base: Hit (no inherit)'	2,166.5 ns	37.267 ns	33.037 ns	0.1221	-	-	384 B
'Method Base: Miss (no inherit)'	331.0 ns	1.943 ns	1.723 ns	-	-	-	-

CustomAttributeData.GetCustomAttributeRecords is the next item I believe, need to think about how to best eliminate that intermediary given the usually small-size arrays it creates. Related question: s there a struct friendly version of ListBuilder<T> that already exists?

jkotas · 2018-11-04T07:15:25Z

have a few more changes to further reduce allocations

I have taken a cursory look and these look great. Thank you!

Related question: s there a struct friendly version of ListBuilder<T> that already exists?

I am not sure what is struct unfriendly in ListBuilder<T>. I see that the T is constrained to class, but it should not be too hard to make it work for valuetypes as well.

NickCraver · 2018-11-04T20:19:22Z

For anyone curious, follow-up posted as #20795

…'s inherited path (dotnet#20779) * Optimization: avoid 2 array allocations in inherited attribute misses This is a follow-up to many allocations recognized in dotnet#20448. A lot of methods ultimately call through CustomAttributes.GetCustomAttributes(). In the inherited search path (default for most searches above), the inheritance path of a class is traversed, resulting in an array allocation per crawled type, a copy to the overall List<object> and then after that - in current code - an array allocation from that list and a typed array allocation it's copied to. However, in the common miss case as simple as: typeof(T).GetCustomAttributes(typeof(TAttr), true); ...and many other overloads, all the same path underneath... We can avoid the last 2 arrays in the miss case. We have the List<object> to go off of. If that's a zero-entry list, we can return an Array.Empty<TAttr>(). That's effectively what this change does. While converting the entire attribute pipeline to generics is problematic and has issue since some object[] return abstracts aren't sealed, we can at least somewhat trivially cache an array per attribute type (only one static, ultimately from Array.Empty<T> underneath) and return that for the miss case. There are far more wins to be had here, but they require more changes. * Move RuntimeType empty array cache generation to Attribute.CreateAttributeArrayHelper This exposes Attribute.CreateAttributeArrayHelper to internal and uses it directly on the RuntimeType caching for empty arrays. Though this allocated 1 additional array overall, it's simpler, faster to init, and still is an infinite win over the per-call allocations before this overall changesets. * CustomAttributes: remove needless array copy in the inherited hit case This removes a .ToArray() for the sake of Array.Copy() where a simple for loop suffices and removes the allocation. Reversing the "empty" result checks is also just a bit cleaner here. This also expands the same fix to the MemberInfo path. Note: should DRY these up too (longstanding issue) - but let's do that in a separate commit for clarity. * GetCusomAttributes: use ListBuilder<object> for inheritance crawls This exposes RuntimeType.ListBuilder<T> for internal usage and replaces the List<T> allocation in GetCustomAttributes() paths to reduce allocations and increase performance in the inherited crawl paths (which is the default for many optional-parameter methods in layers above). Note: there is a subtle behavior depending on previous-null (not possible with a struct now) in AttributeUsageCheck() that I believe still behaves correctly, but could use another set of eyes and a full test suite run to confirm. object[] attributes was removed there simply because it wasn't used before - only cleaning up. * Attribute caching: use Array.CreateInstance() directly on RuntimeType This also reverts the CreateAttributeArrayHelper => internal change, since it's no longer needed. * Ref passing for RuntimeType.ListBuilder<object> & CustomAttribute simplification This fixes the struct passing duplication and tweaks how we're creating arrays a bit, centralizing the zero-element checks to cover all cases as well as simplify the per-method code to rely on the fact this is happening underneath.

This benchmarks specifically GetCustomAttributes and IsDefined (used in dotnet/coreclr#20779 and dotnet/coreclr#20795) Note: there are many code paths through the attribute pipeline which vary drastically in terms of performance and allocations - so there's what seems to be an excessive number of benchmarks here, but they are all pretty unique.

* Adds initial benchmarks for System.Reflection: attributes This benchmarks specifically GetCustomAttributes and IsDefined (used in dotnet/coreclr#20779 and dotnet/coreclr#20795) Note: there are many code paths through the attribute pipeline which vary drastically in terms of performance and allocations - so there's what seems to be an excessive number of benchmarks here, but they are all pretty unique. * Add new category to perflab\ReflectionPerf * Consolidate classes PR feedback: don't follow the Math conventions here...go with what it should be :)

…'s inherited path (dotnet/coreclr#20779) * Optimization: avoid 2 array allocations in inherited attribute misses This is a follow-up to many allocations recognized in dotnet/coreclr#20448. A lot of methods ultimately call through CustomAttributes.GetCustomAttributes(). In the inherited search path (default for most searches above), the inheritance path of a class is traversed, resulting in an array allocation per crawled type, a copy to the overall List<object> and then after that - in current code - an array allocation from that list and a typed array allocation it's copied to. However, in the common miss case as simple as: typeof(T).GetCustomAttributes(typeof(TAttr), true); ...and many other overloads, all the same path underneath... We can avoid the last 2 arrays in the miss case. We have the List<object> to go off of. If that's a zero-entry list, we can return an Array.Empty<TAttr>(). That's effectively what this change does. While converting the entire attribute pipeline to generics is problematic and has issue since some object[] return abstracts aren't sealed, we can at least somewhat trivially cache an array per attribute type (only one static, ultimately from Array.Empty<T> underneath) and return that for the miss case. There are far more wins to be had here, but they require more changes. * Move RuntimeType empty array cache generation to Attribute.CreateAttributeArrayHelper This exposes Attribute.CreateAttributeArrayHelper to internal and uses it directly on the RuntimeType caching for empty arrays. Though this allocated 1 additional array overall, it's simpler, faster to init, and still is an infinite win over the per-call allocations before this overall changesets. * CustomAttributes: remove needless array copy in the inherited hit case This removes a .ToArray() for the sake of Array.Copy() where a simple for loop suffices and removes the allocation. Reversing the "empty" result checks is also just a bit cleaner here. This also expands the same fix to the MemberInfo path. Note: should DRY these up too (longstanding issue) - but let's do that in a separate commit for clarity. * GetCusomAttributes: use ListBuilder<object> for inheritance crawls This exposes RuntimeType.ListBuilder<T> for internal usage and replaces the List<T> allocation in GetCustomAttributes() paths to reduce allocations and increase performance in the inherited crawl paths (which is the default for many optional-parameter methods in layers above). Note: there is a subtle behavior depending on previous-null (not possible with a struct now) in AttributeUsageCheck() that I believe still behaves correctly, but could use another set of eyes and a full test suite run to confirm. object[] attributes was removed there simply because it wasn't used before - only cleaning up. * Attribute caching: use Array.CreateInstance() directly on RuntimeType This also reverts the CreateAttributeArrayHelper => internal change, since it's no longer needed. * Ref passing for RuntimeType.ListBuilder<object> & CustomAttribute simplification This fixes the struct passing duplication and tweaks how we're creating arrays a bit, centralizing the zero-element checks to cover all cases as well as simplify the per-method code to rely on the fact this is happening underneath. Commit migrated from dotnet/coreclr@6d9fc60

jkotas reviewed Nov 3, 2018

View reviewed changes

NickCraver added 3 commits November 3, 2018 09:07

stephentoub approved these changes Nov 3, 2018

View reviewed changes

jkotas reviewed Nov 3, 2018

View reviewed changes

NickCraver added 3 commits November 3, 2018 13:16

Attribute caching: use Array.CreateInstance() directly on RuntimeType

496a760

This also reverts the CreateAttributeArrayHelper => internal change, since it's no longer needed.

Merge upstream master

f6307e3

jkotas approved these changes Nov 3, 2018

View reviewed changes

NickCraver changed the title ~~Optimization: avoid 2 array allocations in GetCustomAtttributes()'s inherited path~~ Optimization: Reduce many array allocations in GetCustomAtttributes()'s inherited path Nov 3, 2018

stephentoub reviewed Nov 3, 2018

View reviewed changes

stephentoub approved these changes Nov 3, 2018

View reviewed changes

Tidy up RuntimeType casts from object

b2ac5c6

jkotas merged commit 6d9fc60 into dotnet:master Nov 4, 2018

NickCraver mentioned this pull request Nov 4, 2018

Optimization: Further reduce allocations in GetCustomAtttributes() paths #20795

Merged

danmoseley mentioned this pull request Dec 6, 2018

Startup cost of XamlServices parsing is 27% slower on .NETCore than on .NET Framework dotnet/wpf#94

Open

NickCraver mentioned this pull request Dec 9, 2018

Addition: Initial Benchmarks for System.Reflection: Attributes dotnet/performance#177

Merged

JasonBock mentioned this pull request Mar 30, 2019

Eliminate hardcoded DataPortal_XYZ method names MarimerLLC/csla#1102

Closed

benaadams mentioned this pull request Oct 25, 2019

Use ValueStringBuilder instead of StringBuidler{Cache} in multiple places #27441

Merged

NickCraver mentioned this pull request Jan 31, 2020

Performance Regression: GetCustomAttributes() Allocations Increased from netcoreapp2.1 dotnet/runtime#11637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NickCraver commented Nov 3, 2018 •

edited

Loading

jkotas Nov 3, 2018

NickCraver Nov 3, 2018

jkotas Nov 3, 2018

NickCraver Nov 3, 2018

jkotas Nov 3, 2018 •

edited

Loading

NickCraver Nov 3, 2018

jkotas Nov 3, 2018

NickCraver Nov 3, 2018

jkotas Nov 3, 2018 •

edited

Loading

jkotas Nov 3, 2018

NickCraver Nov 3, 2018

jkotas Nov 3, 2018

jkotas Nov 3, 2018

NickCraver Nov 3, 2018

NickCraver Nov 3, 2018

NickCraver commented Nov 3, 2018

jkotas left a comment

stephentoub Nov 3, 2018

NickCraver Nov 3, 2018

stephentoub Nov 3, 2018

NickCraver commented Nov 3, 2018

NickCraver commented Nov 4, 2018

jkotas commented Nov 4, 2018

NickCraver commented Nov 4, 2018

		@@ -1294,6 +1294,11 @@ internal static object[] GetCustomAttributes(RuntimeType type, RuntimeType caTyp
		type = type.BaseType as RuntimeType;
		}

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

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

Conversation

NickCraver commented Nov 3, 2018 • edited Loading

Before

Cached Array & loop copy

Above with ref ListBuilder<T> usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickCraver commented Nov 3, 2018

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickCraver commented Nov 3, 2018

NickCraver commented Nov 4, 2018

jkotas commented Nov 4, 2018

NickCraver commented Nov 4, 2018

NickCraver commented Nov 3, 2018 •

edited

Loading

Above with `ref` `ListBuilder<T>` usage

jkotas Nov 3, 2018 •

edited

Loading

jkotas Nov 3, 2018 •

edited

Loading