Analyzer Proposal: Recommend use of concrete types to maximize devirtualization potential

[geeknoid]

Enabling the JIT to devirtualize has a number of benefits as outlined in #49944. To aid in that process, it is preferable to use concrete types rather than interfaces. In particular, private fields, local variables, and private and/or internal method parameters all benefit from being concrete types.

So for example:

private readonly List<string> _myStuff = new ();

is preferable (perf-wise) to:

private readonly IList<string> _myStuff = new List<string>();

By reporting warnings only on private fields, variables, private+internal method parameters, the analyzer would not induce any change in the API surface of a component, where interfaces are often desirable. But it can help speed up the inner-workings of components by fostering devirtualization and hence inlining.

[dotnet-issue-labeler]

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

[aalmada]

I already implemented the rule in my analyzer https://github.com/NetFabric/NetFabric.Hyperlinq.Analyzer/blob/master/docs/reference/HLQ001_AssignmentBoxing.md

[geeknoid]

I already implemented the rule in my analyzer https://github.com/NetFabric/NetFabric.Hyperlinq.Analyzer/blob/master/docs/reference/HLQ001_AssignmentBoxing.md

Yeah, that's the general idea. Although your analysis is specialized for enumerables, I'm proposing something more general.

Basically, if a field, variable or private method parameter is only ever assigned a concrete type, then that element should be declared of that type rather than be declared using an interface type or a base type.

PS: I like your analyzer, I'm going to look at using it in our projects :-)

[JulieLeeMSFT]

CC @AndyAyersMS @dotnet/jit-contrib

[AndyAyersMS]

This doesn't seem like a codegen issue, though having stronger type info definitely will help the jit generate better code.

@stephentoub perhaps you can suggest a more appropriate area label?

[stephentoub]

Technically such a change can introduce behavioral changes, if the public API on the concrete substituted type differs in behavior from the interface it's replacing, e.g. if the field was IDictionary and it was replaced by Dictionary<object, object>, an access site like object value = _dict[key] with a key that didn't exist will start throwing rather than returning null.

But, such an impact should be relatively rare, and it probably just impacts what level we choose to default the rule to, e.g. hidden.

There are other non-devirtualization benefits to this as well, e.g. if you have:

private IList<int> _list = ...;

and then code that enumerates it:

foreach (int i in _list) { ... }

that enumeration will be much more efficient if the field is:

private List<int> _list = ...;

avoiding enumerator allocation, interface dispatch in the enumeration, etc.

[bartonjs]

Video

As far as performance is concerned, the best results happen when this goes from an interface to a sealed type, but there's still goodness in moving up to a more-derived type (and less so for a more-derived interface).

The main identified complication is when a field, auto-property or variable is declared of an interface and changing it to a concrete type causes a compile failure (or runtime behavior) because of explicit interface implementations.

• Look at all assignments into the field or variable
• Find the most specific common type for all of those assignments
• If that type is not the current declaration type, trigger the diagnostic. (And the fixer does that substitution)

Do not run on public or protected fields (or auto-properties) -- or internal if the assembly has any InternalsVisibleTo.

This may, in the case of explicit interface implementations, cause code to change or fail to compile, but we think we're OK with that.

• Category: Performance
• Severity: Information

[carlossanlop]

@dotnet/jit-contrib I marked this as up for grabs, if no one is currently working on this.

@aalmada you mentioned you already had an implementation of this analyzer. Would you like to work on this? (f the members of @dotnet/jit-contrib are ok with that).

[stephentoub]

@geeknoid, do you want to open a PR to dotnet/roslyn-analyzers for this?

[geeknoid]

@geeknoid, do you want to open a PR to dotnet/roslyn-analyzers for this?

Yep, on my todo list :-)

[stephentoub]

Great

[stephentoub]

Implemented in dotnet/roslyn-analyzers#6370