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

NickCraver · 2018-11-04T20:02:24Z

This is a continuation of #20779 optimizing various Attribute allocations.

Two items addressed here (split into 3 commits) are:

PseudoCustomAttribute's intermediate array allocation (used when synthesize Attribute instances from things like [Serializable] under the covers). Previously in the case of any synthesized attributes being present, an Attribute[] array was created to return these to the caller method, and was discarded afterwards. It also DRYs up a lot of repeated code there.
GetCustomAttributes() inner-most methods was doing similar, returning an object[] each time. When crawling up a class's inheritance hierarchy in the inherit: true case, this resulted in an array per parent. Then were then copied to the overall list and the array was discarded. The other overload calling into the core method here actually uses the created object[] array to return it back to the external caller (and remains necessary). The split here adds to the ListBuilder<object> instead for the inner method, and the array is only created when needed in the second caller. So now, when crawling up the hierarchy of parents we just add to the same list each time.
A small bit of cleanup removing unused code.

Note: the GetCustomAttributes() => AddCustomAttributes() split could use careful eyes around the behavior difference of the iteration levels.

Before: the delayed copy into the main list meant that calls into FilterCustomAttributeRecord() had the derivedAttributes list of the previous inheritor at the time of evaluation.
After: now since attributes are added as they are encountered, attributes found at the current inheritance level are included in the FilterCustomAttributeRecord() evaluation.

Looking at FilterCustomAttributeRecord and how it's evaluated, I don't think this matters...but yeah: should be scrutinized. I wouldn't be surprised if tests didn't cover a difference here if there is one that matters.

Based on the same benchmarks as before (source in #20779):

Before

Method	Mean	Error	StdDev	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
'Class: Hit (inherit)'	2,132.5 ns	42.672 ns	55.486 ns	0.1297	-	-	416 B
'Class: Miss (inherit)'	522.8 ns	10.242 ns	10.958 ns	-	-	-	-
'Class: Hit (no inherit)'	1,649.3 ns	15.553 ns	14.548 ns	0.1221	-	-	384 B
'Class: Miss (no inherit)'	229.8 ns	2.511 ns	2.349 ns	-	-	-	-
'Method Override: Hit (inherit)'	3,312.8 ns	24.021 ns	22.470 ns	0.2251	-	-	720 B
'Method Override: Miss (inherit)'	1,945.2 ns	31.742 ns	29.691 ns	0.1297	-	-	416 B
'Method Override: Hit (no inherit)'	1,694.2 ns	6.668 ns	5.911 ns	0.1259	-	-	400 B
'Method Override: Miss (no inherit)'	1,689.9 ns	33.668 ns	71.018 ns	0.1221	-	-	384 B
'Method Base: Hit (inherit)'	1,928.4 ns	25.686 ns	21.449 ns	0.1297	-	-	416 B
'Method Base: Miss (inherit)'	344.0 ns	3.167 ns	2.963 ns	-	-	-	-
'Method Base: Hit (no inherit)'	1,678.6 ns	10.756 ns	8.982 ns	0.1221	-	-	384 B
'Method Base: Miss (no inherit)'	231.9 ns	4.167 ns	3.694 ns	-	-	-	-

After

Method	Mean	Error	StdDev	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
'Class: Hit (inherit)'	1,757.9 ns	10.445 ns	9.259 ns	0.1221	-	-	384 B
'Class: Miss (inherit)'	386.1 ns	2.424 ns	2.267 ns	-	-	-	-
'Class: Hit (no inherit)'	1,585.4 ns	15.031 ns	13.325 ns	0.1221	-	-	384 B
'Class: Miss (no inherit)'	233.5 ns	1.760 ns	1.646 ns	-	-	-	-
'Method Override: Hit (inherit)'	2,663.0 ns	17.049 ns	15.947 ns	0.2060	-	-	656 B
'Method Override: Miss (inherit)'	1,618.0 ns	10.219 ns	9.559 ns	0.1221	-	-	384 B
'Method Override: Hit (no inherit)'	1,602.1 ns	13.484 ns	11.260 ns	0.1259	-	-	400 B
'Method Override: Miss (no inherit)'	1,599.2 ns	7.126 ns	6.666 ns	0.1221	-	-	384 B
'Method Base: Hit (inherit)'	1,621.4 ns	8.812 ns	8.243 ns	0.1221	-	-	384 B
'Method Base: Miss (inherit)'	275.0 ns	1.786 ns	1.671 ns	-	-	-	-
'Method Base: Hit (no inherit)'	1,615.2 ns	16.760 ns	13.995 ns	0.1221	-	-	384 B
'Method Base: Miss (no inherit)'	258.2 ns	2.453 ns	2.175 ns	-	-	-	-

Inside CustomAttribute, PseudoCustomAttribute is the mechanism used to return Attribute instances for what are really flags and such underneath in most cases. This is just an implentation detail, but this detail creates an intermediate array just for passing things around inside CustomAttribute...that's not necessary. This replaces the Attribute[] allocations with the ListBuilder<Attribute> struct for accumulating and relaying this attributes to parent methods. Many overloads had a lot of duplicate coalescing code on this path as well...DRYed up that series of methods. This change reduces allocation overhead for all calls by 40 bytes. In the case of a miss where none would be returned (e.g. .GetCustomAttributes<Attribute>() on a non-attributed class), this reduces allocations down to zero.

…e cases The split here was simple, but allocated an extra intermediate array for each parent type when searching for attributes in the inheritance case. Instead of doing that, we now move that to the second use case exclusively (which uses/returns the array created). The other use case adds to the ListBuilder<object> struct to accumulate through the inheritance hierarchy crawl (rather than an after-the-fact copy). Note some parameter removals on the filter side...those weren't used already (and got in the way of this change).

Just removing the cruft that isn't actually used in here anymore (some of it in quite some time). Trivial amounts of speed gain, but every little bit helps!

NickCraver · 2018-11-04T20:18:43Z

cc @jkotas @stephentoub @benaadams @noahfalk

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

Thsi eats the resize on the > 1 case, but that's so rare it's a net win to start with 0 here.

A bit if a performance boost from the struct copy removal

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

…aths In relation to dotnet#20795, these old calls seem to be for consistency but they're not likely to change and a comment (I think) accomplishes the same goal with less code...so here's a commit demonstrating it.

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

Make it clear that pseudo attributes are a distinct thing (e.g. why not in *this* method) than the return line.

jkotas

Thank you!

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 :)

…ths (dotnet/coreclr#20795) * Remove PseudoCustomAttribute allocations Inside CustomAttribute, PseudoCustomAttribute is the mechanism used to return Attribute instances for what are really flags and such underneath in most cases. This is just an implentation detail, but this detail creates an intermediate array just for passing things around inside CustomAttribute...that's not necessary. This replaces the Attribute[] allocations with the ListBuilder<Attribute> struct for accumulating and relaying this attributes to parent methods. Many overloads had a lot of duplicate coalescing code on this path as well...DRYed up that series of methods. This change reduces allocation overhead for all calls by 40 bytes. In the case of a miss where none would be returned (e.g. .GetCustomAttributes<Attribute>() on a non-attributed class), this reduces allocations down to zero. * CustomAttribute: split GetCustomAttributes into total and additive use cases The split here was simple, but allocated an extra intermediate array for each parent type when searching for attributes in the inheritance case. Instead of doing that, we now move that to the second use case exclusively (which uses/returns the array created). The other use case adds to the ListBuilder<object> struct to accumulate through the inheritance hierarchy crawl (rather than an after-the-fact copy). Note some parameter removals on the filter side...those weren't used already (and got in the way of this change). * CustomAttributes: Cleanup of unused ops Just removing the cruft that isn't actually used in here anymore (some of it in quite some time). Trivial amounts of speed gain, but every little bit helps! * Param typo and red passing for GetCombinedList() * Assume zero pseudo attributes by default (the most likely case) Thsi eats the resize on the > 1 case, but that's so rare it's a net win to start with 0 here. * Pseudo attributes: use out instead of return A bit if a performance boost from the struct copy removal * Pseudo attributes: switch to out everywhere and remove uneeded code paths In relation to dotnet/coreclr#20795, these old calls seem to be for consistency but they're not likely to change and a comment (I think) accomplishes the same goal with less code...so here's a commit demonstrating it. * Comment spacing for pseudo attribute fetching Make it clear that pseudo attributes are a distinct thing (e.g. why not in *this* method) than the return line. Commit migrated from dotnet/coreclr@477c252

NickCraver added 3 commits November 4, 2018 11:26

CustomAttributes: Cleanup of unused ops

1d56cce

Just removing the cruft that isn't actually used in here anymore (some of it in quite some time). Trivial amounts of speed gain, but every little bit helps!

NickCraver changed the title ~~Optimization: Further reduce many allocations in GetCustomAtttributes() paths~~ Optimization: Further reduce allocations in GetCustomAtttributes() paths Nov 4, 2018

NickCraver mentioned this pull request Nov 4, 2018

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

Merged

jkotas reviewed Nov 5, 2018

View reviewed changes