Avoid allocations in CanWrap... methods

[martin-strecker-sonarsource]

The "CanWrap.." methods are in the hot path in our repository (see SonarSource/sonar-dotnet/issues/8106). It is likely that the methods are on the hot path in your analyzer as well (they are called by each ...Wrapper.IsInstance implementation).

The following improvements can be validated by the generated IL:

• Capture class creation is removed: IL_00000 creates a capture class for the whole method. In the new version, no capture class is created.
• Func<,> delegate instantiation for _ => new ConcurrentDictionary<Type, bool>() is deferred for sure. The function delegate in IL_0047 seems to be stored in a hidden static field and reused in the old version. The new version makes sure that the delegate is not instantiated on the happy path for sure. IL_0033 in the new version is only conditional executed after the branch after the SupportedSyntaxWrappers.TryGetValue (IL_0017) and a second branch after the static field lookup (IL_0025).
• If wrappedSyntax.TryGetValue is not successful canCast is calculated and added to the ConcurrentDictionary without a delegate. This is possible because canCast is a value type. Unlike with reference types, the created value doesn't suffer from concurrency issues if TryAdd fails.

The allocation saving for the capture class was measured in this SonarSource/sonar-dotnet/pull/8107 on our side with the similar changes in place.

The IL for the old version of CanWrapNode looks like so (3 times newobj):

  .method assembly hidebysig static bool
    CanWrapNode(
      class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode node,
      class [System.Runtime]System.Type underlyingType
    ) cil managed
  {
    .maxstack 4
    .locals init (
      [0] class StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0' 'CS$<>8__locals0',
      [1] class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool> wrappedSyntax,
      [2] bool canCast
    )

    IL_0000: newobj       instance void StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::.ctor()
    IL_0005: stloc.0      // 'CS$<>8__locals0'
    IL_0006: ldloc.0      // 'CS$<>8__locals0'
    IL_0007: ldarg.1      // underlyingType
    IL_0008: stfld        class [System.Runtime]System.Type StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::underlyingType
    IL_000d: ldloc.0      // 'CS$<>8__locals0'
    IL_000e: ldarg.0      // node
    IL_000f: stfld        class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::node

    // [84 13 - 84 30]
    IL_0014: ldloc.0      // 'CS$<>8__locals0'
    IL_0015: ldfld        class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::node
    IL_001a: brtrue.s     IL_001e

    // [87 17 - 87 29]
    IL_001c: ldc.i4.1
    IL_001d: ret

    // [90 13 - 90 40]
    IL_001e: ldloc.0      // 'CS$<>8__locals0'
    IL_001f: ldfld        class [System.Runtime]System.Type StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::underlyingType
    IL_0024: brtrue.s     IL_0028

    // [93 17 - 93 30]
    IL_0026: ldc.i4.0
    IL_0027: ret

    // [96 13 - 96 168]
    IL_0028: ldsfld       class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers::SupportedSyntaxWrappers
    IL_002d: ldloc.0      // 'CS$<>8__locals0'
    IL_002e: ldfld        class [System.Runtime]System.Type StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::underlyingType
    IL_0033: ldsfld       class [System.Runtime]System.Func`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9__30_0'
    IL_0038: dup
    IL_0039: brtrue.s     IL_0052
    IL_003b: pop
    IL_003c: ldsfld       class StyleCop.Analyzers.Lightup.LightupHelpers/'<>c' StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9'
    IL_0041: ldftn        instance class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<CanWrapNode>b__30_0'(class [System.Runtime]System.Type)
    IL_0047: newobj       instance void class [System.Runtime]System.Func`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>>::.ctor(object, native int)
    IL_004c: dup
    IL_004d: stsfld       class [System.Runtime]System.Func`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9__30_0'
    IL_0052: callvirt     instance !1/*class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>*/ class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>>::GetOrAdd(!0/*class [System.Runtime]System.Type*/, class [System.Runtime]System.Func`2<!0/*class [System.Runtime]System.Type*/, !1/*class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>*/>)
    IL_0057: stloc.1      // wrappedSyntax

    // [100 13 - 100 70]
    IL_0058: ldloc.1      // wrappedSyntax
    IL_0059: ldloc.0      // 'CS$<>8__locals0'
    IL_005a: ldfld        class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::node
    IL_005f: call         valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.CSharpExtensions::Kind(class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode)
    IL_0064: ldloca.s     canCast
    IL_0066: callvirt     instance bool class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>::TryGetValue(!0/*valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind*/, !1/*bool*/&)
    IL_006b: brtrue.s     IL_008b

    // [102 17 - 104 106]
    IL_006d: ldloc.1      // wrappedSyntax
    IL_006e: ldloc.0      // 'CS$<>8__locals0'
    IL_006f: ldfld        class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::node
    IL_0074: call         valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.CSharpExtensions::Kind(class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode)
    IL_0079: ldloc.0      // 'CS$<>8__locals0'
    IL_007a: ldftn        instance bool StyleCop.Analyzers.Lightup.LightupHelpers/'<>c__DisplayClass30_0'::'<CanWrapNode>b__1'(valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind)
    IL_0080: newobj       instance void class [System.Runtime]System.Func`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>::.ctor(object, native int)
    IL_0085: callvirt     instance !1/*bool*/ class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>::GetOrAdd(!0/*valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind*/, class [System.Runtime]System.Func`2<!0/*valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind*/, !1/*bool*/>)
    IL_008a: stloc.2      // canCast

    // [107 13 - 107 28]
    IL_008b: ldloc.2      // canCast
    IL_008c: ret

  } // end of method LightupHelpers::CanWrapNode

The IL for the new version looks like so (1 time newobj):

  .method assembly hidebysig static bool
    CanWrapNode(
      class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode node,
      class [System.Runtime]System.Type underlyingType
    ) cil managed
  {
    .maxstack 4
    .locals init (
      [0] class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool> wrappedSyntax,
      [1] bool canCast
    )

    // [82 13 - 82 30]
    IL_0000: ldarg.0      // node
    IL_0001: brtrue.s     IL_0005

    // [85 17 - 85 29]
    IL_0003: ldc.i4.1
    IL_0004: ret

    // [88 13 - 88 40]
    IL_0005: ldarg.1      // underlyingType
    IL_0006: brtrue.s     IL_000a

    // [91 17 - 91 30]
    IL_0008: ldc.i4.0
    IL_0009: ret

    // [94 13 - 94 175]
    IL_000a: ldsfld       class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers::SupportedSyntaxWrappers
    IL_000f: ldarg.1      // underlyingType
    IL_0010: ldsfld       class [System.Runtime]System.Func`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9__30_0'
    IL_0015: dup
    IL_0016: brtrue.s     IL_002f
    IL_0018: pop
    IL_0019: ldsfld       class StyleCop.Analyzers.Lightup.LightupHelpers/'<>c' StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9'
    IL_001e: ldftn        instance class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<CanWrapNode>b__30_0'(class [System.Runtime]System.Type)
    IL_0024: newobj       instance void class [System.Runtime]System.Func`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>>::.ctor(object, native int)
    IL_0029: dup
    IL_002a: stsfld       class [System.Runtime]System.Func`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9__30_0'
    IL_002f: callvirt     instance !1/*class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>*/ class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>>::GetOrAdd(!0/*class [System.Runtime]System.Type*/, class [System.Runtime]System.Func`2<!0/*class [System.Runtime]System.Type*/, !1/*class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>*/>)
    IL_0034: stloc.0      // wrappedSyntax

    // [97 13 - 97 74]
    IL_0035: ldloc.0      // wrappedSyntax
    IL_0036: ldarg.0      // node
    IL_0037: call         valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.CSharpExtensions::Kind(class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode)
    IL_003c: ldloca.s     canCast
    IL_003e: callvirt     instance bool class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>::TryGetValue(!0/*valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind*/, !1/*bool*/&)
    IL_0043: brtrue.s     IL_006a

    // [99 17 - 99 103]
    IL_0045: ldarg.1      // underlyingType
    IL_0046: call         class [System.Reflection]System.Reflection.TypeInfo [System.Reflection]System.Reflection.IntrospectionExtensions::GetTypeInfo(class [System.Runtime]System.Type)
    IL_004b: ldarg.0      // node
    IL_004c: callvirt     instance class [System.Runtime]System.Type [System.Runtime]System.Object::GetType()
    IL_0051: call         class [System.Reflection]System.Reflection.TypeInfo [System.Reflection]System.Reflection.IntrospectionExtensions::GetTypeInfo(class [System.Runtime]System.Type)
    IL_0056: callvirt     instance bool [System.Reflection]System.Reflection.TypeInfo::IsAssignableFrom(class [System.Reflection]System.Reflection.TypeInfo)
    IL_005b: stloc.1      // canCast

    // [100 17 - 100 60]
    IL_005c: ldloc.0      // wrappedSyntax
    IL_005d: ldarg.0      // node
    IL_005e: call         valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.CSharpExtensions::Kind(class [Microsoft.CodeAnalysis]Microsoft.CodeAnalysis.SyntaxNode)
    IL_0063: ldloc.1      // canCast
    IL_0064: callvirt     instance bool class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>::TryAdd(!0/*valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind*/, !1/*bool*/)
    IL_0069: pop

    // [103 13 - 103 28]
    IL_006a: ldloc.1      // canCast
    IL_006b: ret

  } // end of method LightupHelpers::CanWrapNode

[codecov]

Codecov Report

Merging #3711 (b76cba6) into master (0548969) will increase coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head b76cba6 differs from pull request most recent head 12b6f93. Consider uploading reports for the commit 12b6f93 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3711      +/-   ##
==========================================
+ Coverage   93.12%   93.13%   +0.01%     
==========================================
  Files        1088     1088              
  Lines      114954   115026      +72     
  Branches     4088     4113      +25     
==========================================
+ Hits       107047   107126      +79     
+ Misses       6865     6860       -5     
+ Partials     1042     1040       -2     

[bjornhellander]

Is it still benefitial to use GetOrAdd after having checked with TryGetValue, or would it be even better to simply call TryAdd instead, to not need to capture any variables at all and get rid of the local function?

[bjornhellander]

Answering myself: Since the local function in your code is rarely called, I assume there will be no practical difference in performance. But maybe it would be better with the simpler code, for readability?

...

if (!wrappedObject.TryGetValue(obj.GetType(), out var canCast))
{
    canCast = underlyingType.GetTypeInfo().IsAssignableFrom(obj.GetType().GetTypeInfo());
    wrappedObject.TryAdd(obj.GetType(), canCast);
}

return canCast;

Just a thought.

[bjornhellander]

Was there an actual problem with this first lookup (in each method)? I haven't dug into your IL comparison, but since no capture is needed, it seems the only possible overhead would be creating the func instance. I think I recall that lambdas are automatically detected to be static if possible, but in case that is not correct, wouldn't simply marking the lambda as static be the best change?

ConcurrentDictionary<Type, bool> wrappedObject = SupportedObjectWrappers.GetOrAdd(underlyingType, static _ => new ConcurrentDictionary<Type, bool>());

[martin-strecker-sonarsource]

The IL confirmed that the delegate instance is indeed cached between calls in a static field:

// Load the delegate instance from a static field  
    IL_0033: ldsfld       class System.Func`2<System.Type, ConcurrentDictionary`2<SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9__30_0'
    IL_0038: dup
// Branch if not null?
    IL_0039: brtrue.s     IL_0052
    IL_003b: pop
// ..
// Create the delegate
    IL_0047: newobj       instance void class System.Func`2<System.Type, ConcurrentDictionary`2<SyntaxKind, bool>>::.ctor(object, native int)
    IL_004c: dup
// Store it
    IL_004d: stsfld       class System.Func`2<System.Type, ConcurrentDictionary`2<SyntaxKind, bool>> StyleCop.Analyzers.Lightup.LightupHelpers/'<>c'::'<>9__30_0'
// Invoke the delegate
    IL_0052: callvirt     instance !1/*class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>*/ class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<class [System.Runtime]System.Type, class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>>::GetOrAdd(!0/*class [System.Runtime]System.Type*/, class [System.Runtime]System.Func`2<!0/*class [System.Runtime]System.Type*/, !1/*class [System.Collections.Concurrent]System.Collections.Concurrent.ConcurrentDictionary`2<valuetype [Microsoft.CodeAnalysis.CSharp]Microsoft.CodeAnalysis.CSharp.SyntaxKind, bool>*/>)

[martin-strecker-sonarsource]

But maybe it would be better with the simpler code, for readability?

This is indeed better. It works in this case because the cached value is a bool (value type). For the SupportedObjectWrappers case it wouldn't work as well as we would need to do another TryGetValue if TryAdd is failing (the ConcurrentDictionary we just created is not the same as the one stored by another thread in the meantime in the ConcurrentDictionary).

[sharwell]

Requesting simplification

[sharwell]

📝 This line does not need to change.

[martin-strecker-sonarsource]

I just kept the static to emphasize the non-capturing semantic of the lambda.

[sharwell]

📝 This comment is sufficient for the method (it already states the complete intent, and it's just a simple bug that we didn't adhere to it)

[sharwell]

📝 This line is the source of the capturing allocation (specifically, capturing a method parameter in a lambda forces the the capture to be allocated on all paths and not just the one that creates this delegate).

[sharwell]

❗ Requesting that this pull request be reduced to just this two line change in each case.

[martin-strecker-sonarsource]

@sharwell Merging failed due to some (unrelated?) build error.

[bjornhellander]

I consider myself unlucky when it comes to these builds, but you are on another level :-) I think that you got all the "usual" random build errors plus one that I don't think I have ever seen. There are however some new commits on master, so you can merge those and push again. Maybe fifth time is the charm? Otherwise I hope that @sharwell can merge this anyway.

[sharwell]

I'll just restart the build. Merging from master will turn off the auto merge.