Optimise GetCustomAttribute(s)

[benaadams]

For GetCustomAttribute brings allocations down to 25% and down to 3% for #44713 (to just the requested attribute); with a performance improvement of 25% and 56% for #44713

For PropertyInfo, Type, ConstructorInfo, MethodInfo, FieldInfo, EventInfo

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	True	GetCustomAttribute	1,000.4 ns	664 B	1.00
PropertyInfo	\pr\corerun.exe	True	GetCustomAttribute	431.2 ns	24 B	0.43
PropertyInfo	now	True	GetCustomAttribute	368.5 ns	24 B	0.37
Type	\master\corerun.exe	True	GetCustomAttribute	714.6 ns	104 B	1.00
Type	\pr\corerun.exe	True	GetCustomAttribute	515.3 ns	24 B	0.72
Type	now	True	GetCustomAttribute	450.5 ns	24 B	0.63
ConstructorInfo	\master\corerun.exe	True	GetCustomAttribute	576.3 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	True	GetCustomAttribute	419.7 ns	24 B	0.73
ConstructorInfo	now	True	GetCustomAttribute	350.0 ns	24 B	0.61
MethodInfo	\master\corerun.exe	True	GetCustomAttribute	611.6 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	True	GetCustomAttribute	462.9 ns	24 B	0.76
MethodInfo	now	True	GetCustomAttribute	392.2 ns	24 B	0.64
FieldInfo	\master\corerun.exe	True	GetCustomAttribute	618.4 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	True	GetCustomAttribute	436.7 ns	24 B	0.71
FieldInfo	now	True	GetCustomAttribute	380.7 ns	24 B	0.61
EventInfo	\master\corerun.exe	True	GetCustomAttribute	1,034.5 ns	664 B	1.00
EventInfo	\pr\corerun.exe	True	GetCustomAttribute	471.0 ns	24 B	0.46
EventInfo	now	True	GetCustomAttribute	408.2 ns	24 B	0.39

For GetCustomAttributes and GetCustomAttributes(Type) it halves allocations to just array + attribute

Resolves #44713

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[jkotas]

Is this fixing #44713 ?

[GrabYourPitchforks]

I don't understand the context for this PR. What is it attempting to optimize, and how does it accomplish the optimization?

[benaadams]

Calling GetCustomAttribute singlar calls GetCustomAttributes plural to create an array of attributes which it then throws away returning one item (or throwing if there are more than one item); the optimization is to skip the intermediate array creation.

[benaadams]

Is this fixing #44713 ?

Ish; though that's PropertyInfo, which I haven't done; as its only RuntimeType, RuntimeMethodInfo

Will add the rest of MemberInfo's derived types RuntimeEventInfo, RuntimeFieldInfo, RuntimePropertyInfo, RuntimeConstructorInfo which will then fix that issue.

[benaadams]

Working through some issues with ILLink...

[benaadams]

Need to investigate why System.Configuration.ConfigurationManager.Tests and System.Text.Json.Tests are timing out across many variants

Meanwhile some results:

GetCustomAttribute which I was originally looking at; brings allocations down to 25% and down to 3% for #44713 (to just the requested attribute); with a performance improvement of 25% and 56% for #44713

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	False	GetCustomAttribute	562.5 ns	104 B	1.00
PropertyInfo	\pr\corerun.exe	False	GetCustomAttribute	423.8 ns	24 B	0.75
Type	\master\corerun.exe	False	GetCustomAttribute	580.1 ns	104 B	1.00
Type	\pr\corerun.exe	False	GetCustomAttribute	426.5 ns	24 B	0.74
ConstructorInfo	\master\corerun.exe	False	GetCustomAttribute	569.9 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	False	GetCustomAttribute	420.1 ns	24 B	0.74
MethodInfo	\master\corerun.exe	False	GetCustomAttribute	610.0 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	False	GetCustomAttribute	467.4 ns	24 B	0.77
FieldInfo	\master\corerun.exe	False	GetCustomAttribute	590.1 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	False	GetCustomAttribute	440.7 ns	24 B	0.75
EventInfo	\master\corerun.exe	False	GetCustomAttribute	583.7 ns	104 B	1.00
EventInfo	\pr\corerun.exe	False	GetCustomAttribute	420.9 ns	24 B	0.72

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	True	GetCustomAttribute	1,000.4 ns	664 B	1.00
PropertyInfo	\pr\corerun.exe	True	GetCustomAttribute	431.2 ns	24 B	0.43
Type	\master\corerun.exe	True	GetCustomAttribute	714.6 ns	104 B	1.00
Type	\pr\corerun.exe	True	GetCustomAttribute	515.3 ns	24 B	0.72
ConstructorInfo	\master\corerun.exe	True	GetCustomAttribute	576.3 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	True	GetCustomAttribute	419.7 ns	24 B	0.73
MethodInfo	\master\corerun.exe	True	GetCustomAttribute	611.6 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	True	GetCustomAttribute	462.9 ns	24 B	0.76
FieldInfo	\master\corerun.exe	True	GetCustomAttribute	618.4 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	True	GetCustomAttribute	436.7 ns	24 B	0.71
EventInfo	\master\corerun.exe	True	GetCustomAttribute	1,034.5 ns	664 B	1.00
EventInfo	\pr\corerun.exe	True	GetCustomAttribute	471.0 ns	24 B	0.46

[benaadams]

GetCustomAttributes and GetCustomAttributes(Type) are mostly within error rate (due to GC); but allocations halved to just array + attribute

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	False	GetCustomAttributes	500.9 ns	104 B	1.00
PropertyInfo	\pr\corerun.exe	False	GetCustomAttributes	505.0 ns	56 B	1.01
Type	\master\corerun.exe	False	GetCustomAttributes	506.7 ns	104 B	1.00
Type	\pr\corerun.exe	False	GetCustomAttributes	504.0 ns	56 B	0.99
ConstructorInfo	\master\corerun.exe	False	GetCustomAttributes	493.3 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	False	GetCustomAttributes	505.3 ns	56 B	1.02
MethodInfo	\master\corerun.exe	False	GetCustomAttributes	514.4 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	False	GetCustomAttributes	528.6 ns	56 B	1.03
FieldInfo	\master\corerun.exe	False	GetCustomAttributes	611.4 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	False	GetCustomAttributes	616.3 ns	56 B	1.01
EventInfo	\master\corerun.exe	False	GetCustomAttributes	513.8 ns	104 B	1.00
EventInfo	\pr\corerun.exe	False	GetCustomAttributes	507.3 ns	56 B	0.99

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	True	GetCustomAttributes	493.9 ns	104 B	1.00
PropertyInfo	\pr\corerun.exe	True	GetCustomAttributes	526.5 ns	56 B	1.07
Type	\master\corerun.exe	True	GetCustomAttributes	600.2 ns	104 B	1.00
Type	\pr\corerun.exe	True	GetCustomAttributes	604.4 ns	56 B	1.01
ConstructorInfo	\master\corerun.exe	True	GetCustomAttributes	499.4 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	True	GetCustomAttributes	498.2 ns	56 B	1.00
MethodInfo	\master\corerun.exe	True	GetCustomAttributes	539.0 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	True	GetCustomAttributes	594.1 ns	56 B	1.00
FieldInfo	\master\corerun.exe	True	GetCustomAttributes	601.6 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	True	GetCustomAttributes	603.5 ns	56 B	1.00
EventInfo	\master\corerun.exe	True	GetCustomAttributes	503.4 ns	104 B	1.00
EventInfo	\pr\corerun.exe	True	GetCustomAttributes	514.7 ns	56 B	1.02

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	False	GetCu(...)_Type [24]	538.5 ns	104 B	1.00
PropertyInfo	\pr\corerun.exe	False	GetCu(...)_Type [24]	551.2 ns	56 B	1.02
Type	\master\corerun.exe	False	GetCu(...)_Type [24]	558.0 ns	104 B	1.00
Type	\pr\corerun.exe	False	GetCu(...)_Type [24]	570.5 ns	56 B	1.02
ConstructorInfo	\master\corerun.exe	False	GetCu(...)_Type [24]	540.5 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	False	GetCu(...)_Type [24]	544.7 ns	56 B	0.99
MethodInfo	\master\corerun.exe	False	GetCu(...)_Type [24]	589.0 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	False	GetCu(...)_Type [24]	579.0 ns	56 B	0.98
FieldInfo	\master\corerun.exe	False	GetCu(...)_Type [24]	574.6 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	False	GetCu(...)_Type [24]	587.1 ns	56 B	1.02
EventInfo	\master\corerun.exe	False	GetCu(...)_Type [24]	550.9 ns	104 B	1.00
EventInfo	\pr\corerun.exe	False	GetCu(...)_Type [24]	549.5 ns	56 B	1.00

Method	Toolchain	Inherit	CustomAttributeVariant	Mean	Allocated	Ratio
PropertyInfo	\master\corerun.exe	True	GetCu(...)_Type [24]	564.9 ns	104 B	1.00
PropertyInfo	\pr\corerun.exe	True	GetCu(...)_Type [24]	559.7 ns	56 B	0.99
Type	\master\corerun.exe	True	GetCu(...)_Type [24]	668.4 ns	104 B	1.00
Type	\pr\corerun.exe	True	GetCu(...)_Type [24]	638.0 ns	56 B	0.95
ConstructorInfo	\master\corerun.exe	True	GetCu(...)_Type [24]	540.0 ns	104 B	1.00
ConstructorInfo	\pr\corerun.exe	True	GetCu(...)_Type [24]	548.8 ns	56 B	1.02
MethodInfo	\master\corerun.exe	True	GetCu(...)_Type [24]	595.8 ns	104 B	1.00
MethodInfo	\pr\corerun.exe	True	GetCu(...)_Type [24]	605.6 ns	56 B	1.02
FieldInfo	\master\corerun.exe	True	GetCu(...)_Type [24]	569.8 ns	104 B	1.00
FieldInfo	\pr\corerun.exe	True	GetCu(...)_Type [24]	574.6 ns	56 B	1.01
EventInfo	\master\corerun.exe	True	GetCu(...)_Type [24]	566.8 ns	104 B	1.00
EventInfo	\pr\corerun.exe	True	GetCu(...)_Type [24]	565.4 ns	56 B	1.00

[benaadams]

Benchmark code https://gist.github.com/benaadams/3f7ba86da58d253f90fc1891c43f9c64

[benaadams]

Was an infinite loop from not using the updated variable. Will squash and rebase to resolve conflict

[jeffhandley]

The merge conflicts are a bit frightening

@benaadams By frightening, did you mean you could use help resolving them? Or did @ViktorHofer's comment address your concerns?

Thanks!

[jeffhandley]

@benaadams Would you like help resolving the conflicts, or will you be able to? Thanks!

[benaadams]

@jeffhandley can't currently build anything using .NET6.0 in VS dotnet/sdk#17461

[jeffhandley]

@benaadams I worked through the conflicts:

1. Rewrote history for the branch to handle the src/coreclr/src --> src/coreclr folder rename
2. Rebased on main
3. Resolved conflicts from Faster EventSource attribute lookup #45621, which was broken out as a separate PR from this one
4. Resolved a few other conflicts
5. Updated a file where the usings were not sorted, and fixed a typo in one of the new comments

I pushed all of this to a branch on my fork; you can see it here. I've run the build and Reflection unit tests locally, and they're passing. If you'd like, I could force push this into your fork on this branch. Just let me know.

[benaadams]

Please do 😀

[jeffhandley]

Force-pushed. Your unmodified branch is also pushed to my fork in case we need to reference the original for some reason. I'll plan to delete that after we merge this PR.

[jeffhandley]

LGTM, but I'd like a re-review from either @jkotas or @steveharter before we merge it.

[jkotas]

I am not fan of all these new internal methods on abstract base types.

[jkotas]

This change is regressing handling of non-CLS compliant attributes.

using System;
using System.Reflection;
using System.Reflection.Emit;

public class MyAttribute
{
}

class Program
{
    static void Main(string[] args)
    {
        var ab = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("MyAssembly"), AssemblyBuilderAccess.Run);
        var mb = ab.DefineDynamicModule("MyModule");
        var tb = mb.DefineType("MyType");
        tb.SetCustomAttribute(typeof(MyAttribute).GetConstructor(new Type[0]), new byte[0]);
        var t = tb.CreateType();
        foreach (var a in t.GetCustomAttributes(true)) Console.WriteLine(a.ToString());
    }
}

This succeeds before this change, but fails with Unable to cast object of type 'MyAttribute' to type 'System.Attribute' after this change.

[jkotas]

I have been going through this change for a while and I find it difficult to review. It is really multiple independent changes together:

• Changing Dictionary to HashSet
• Changing == / != to is / is not
• Reducing allocations for attribute enumeration
• Faster lookup of attribute usage attribute
• Faster lookup of single attributes

I think it would be more productive to submit these changes one at a time in individual PRs. They would be much easier to review and merge with confidence.

[jkotas]

I do like the idea behind this set of changes. It would be nice to get it in.

[GrabYourPitchforks]

I think it would be more productive to submit these changes one at a time in individual PRs. They would be much easier to review and merge with confidence.

I agree with Jan on this. I know you've been putting considerable care into this PR and it's frustrating to have to split this, but it seems like the fastest way forward.

[jkotas]

I have peeled off a subset of this change into #53152

[jeffhandley]

I've converted this to a draft since per the comments about splitting it up into separate PRs and #53152 already being addressed.

@benaadams -- let us know if you plan to peel off other increments from this or if this should go onto our team's backlog. Thanks!

[ghost]

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.