Cache RetargetingParameterSymbol.TypeWithAnnotations to avoid ~8.3% allocations in a trace

[Youssef1313]

In the trace from https://developercommunity.visualstudio.com/t/Performance-trace-for-Roslyn-team/10413054, there are too many TypeWithAnnotations[] allocations happening in RetargetingParameterSymbol (thanks @sharwell for the investigation).

I re-profiled the scenario with a locally built version of the compiler that adds some logging to CodeAnalysisEventSource, and @sharwell figured out that on average, there are 76 calls to the exact same RetargetingParameterSymbol instance. So we think caching might be beneficial here.

I don't know if that will solve the problem completely, we might need to get another trace with the change from this PR and see if there are more bottlenecks, but at least, this seems like a good first step.

Edit:

The previous screenshot from trace shows 3.5% allocations for TypewithAnnotions arrays, however, there is also the following in the same trace:

which is 4.8% of allocations. So total is 8.3%, and there are probably more allocations happening during the evaluation of this property.

[cston]

Nullable<TypeWithAnnotations> is not thread-safe. Is this field accessed from multiple threads?

[Youssef1313]

@cston Yeah I was chatting with Sam about this. I didn't find an appropriate existing helper, so Sam is working on merging helpers that are not available in the compilers layer with those available in the compilers layer.

[333fred]

No additional helpers are necessary, that's what https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/TypeWithAnnotations.cs,e5078e3d5fd03b05,references is for.

[cston]

If the goal is to avoid recalculating the type, it may be sufficient to just cache the TypeSymbol.

private TypeSymbol? _lazyType;

public sealed override TypeWithAnnotations TypeWithAnnotations
{
    get
    {
        var underlyingType = _underlyingParameter.TypeWithAnnotations;
        var translator = RetargetingModule.RetargetingTranslator;
        if (_lazyType is null)
        {
            Interlocked.CompareExchange(ref _lazyType,
                translator.Retarget(underlyingType.Type,
                    RetargetOptions.RetargetPrimitiveTypesByTypeCode),
                null);
        }
        var newModifiers = translator.RetargetModifiers(underlyingType.CustomModifiers, out _);
        return underlyingType.WithTypeAndModifiers(_lazyType, newModifiers);
    }
}

[Youssef1313]

@cston In the trace I provided, this property is accessed 12,833,732 times during 3 minutes. So I think it's worth to just cache the whole thing.

[Youssef1313]

No additional helpers are necessary, that's what https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/TypeWithAnnotations.cs,e5078e3d5fd03b05,references is for.

Does this suggest it like that?

if (_lazyTypeWithAnnotations is null)
{
    Interlocked.CompareExchange(ref _lazyTypeWithAnnotations, new TypeWithAnnotations.Boxed(......), null);
}

I think that would still risk calculating it twice if two threads concurrently accessed the property.

What Sam suggested was something to be like this:

return LazyInitialization.EnsureInitialized(
    ref _lazyTypeWithAnnotations,
    static self => self.RetargetingModule.RetargetingTranslator.Retarget(self._underlyingParameter.TypeWithAnnotations, RetargetOptions.RetargetPrimitiveTypesByTypeCode),
    this);

[sharwell]

@333fred The helpers are lazy initialization helpers for reference types, and are unrelated to the fact that this is a value type. Even after switching to TypeWithAnnotations.Boxed, we'd want to use the helper method for clarity. See #69017

[AlekseyTs]

We should switch to TypeWithAnnotations.Boxed, which we use to cache this information in symbols. There should be many examples. No special helpers are necessary and the change shouldn't be blocked on that.

[Youssef1313]

I used TypeWithAnnotations.Boxed for now. After #69017 is merged, I can switch to another helper if that would be better.

[333fred]

I would like to better understand the scenario causing an issue here. Can you share the code being compiled?

[Youssef1313]

@333fred The code I'm currently testing with is a private repo. I don't think I'll be able to share it. But I think the repro from #67926 will do the job. It's almost the same idea. The PR #68316 did have a good impact on this scenario, but it doesn't resolve it completely.

It's essentially a codebase full of generic extension methods. In a 3 minute trace, RetargetingParameterSymbol.TypeWithAnnotations was accessed over 10 million times, to be accurate, 12,833,732.

[Youssef1313]

@333fred In case you wanted me to log more events into the trace, just let me know.

[AlekseyTs]

Done with review pass (commit 1)

[sharwell]

In addition to the number of times TypeWithAnnotations is accessed, we showed that on average the TypeWithAnnotations property is accessed 76 times for each RetargetingParameterSymbol instance.

[sharwell]

Blocked on #69017

[333fred]

In a 3 minute trace, RetargetingParameterSymbol.TypeWithAnnotations was accessed over 10 million times, to be accurate, 12,833,732.

I want to understand the codepath that caused this. This seems extreme and like some component is misbehaving. I'm sympathetic to the issue, but I don't want to patch over the problem, I want to understand why it is a problem.

[Youssef1313]

@333fred Does the stacks in PerfView not give enough info about the code path causing this?

Note that the scenario is really full of generic extension methods, even ones with multiple overloads. The IDE lookup requests goes through this loop, which keeps creating reduced extension method symbols

https://github.com/dotnet/roslyn/blob/7c6d5b92a584f0c512b51a0c44a9f441a9443f6f/src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs#L1670C39-L1670C106

In ReducedExtensionMethodSymbol.Create, the type of the first parameter is accessed.

I'd like to add this:

Apparently, this property is taking 8.2% of CPU time.

[AlekseyTs]

Blocked on #69017

Why is that? Regular Interlocked.CompareExchange just works with TypeWithAnnotations.Boxed.

[AlekseyTs]

LGTM (commit 2)

[AlekseyTs]

@dotnet/roslyn-compiler For the second review

[CyrusNajmabadi]

Do you have a trace showing the 'after' results? Thanks!

[Youssef1313]

@CyrusNajmabadi Sure. I uploaded it to the same feedback ticket on DC.

[Youssef1313]

@333fred Given the before/after traces, can this move forward and get merged?

[333fred]

I'm satisfied by scenario here.