@333fred @jcouv Please take a look when you have the chance.

@333fred @jcouv Please review

Looking

Short meeting notes (5/18/2021): Aleksey, Fred, Rikki and I thoroughly explored options, including an approach tracking changed slots. We're thinking that the double-visit approach is still likely the best trade-off (usability/complexity/performance), and we'll do some measurement to validate performance behavior.

Have briefly investigated the performance with various complex right-hand sides.

[SimpleJob(RuntimeMoniker.NetCoreApp50)]
public class ComplexCondAccessEquals
{
    private MetadataReference[] coreRefs = null!;

    [GlobalSetup]
    public void Setup()
    {
        coreRefs = new[] { MetadataReference.CreateFromFile(typeof(object).Assembly.Location) };
    }

    [Params(10, 12, 15, 18)]
    public int Depth { get; set; }

    [Params("enable", "disable")]
    public string NullableContext { get; set; } = null!;

    [Benchmark]
    public void CondAccess_ComplexRightSide()
    {
        var source1 = @"
#nullable " + NullableContext + @"
object? x = null;
C? c = null;
if (
";
        var source2 = @"
)
{
}

class C
{
public bool M(object? obj) => false;
}
";
        var sourceBuilder = new StringBuilder();
        sourceBuilder.Append(source1);
        for (var i = 0; i < Depth; i++)
        {
            sourceBuilder.AppendLine($"    c?.M(x = {i}) == (");
        }
        sourceBuilder.AppendLine("    c!.M(x)");

        sourceBuilder.Append("    ");
        for (var i = 0; i < Depth; i++)
        {
            sourceBuilder.Append(")");
        }

        sourceBuilder.Append(source2);

        var tree = SyntaxFactory.ParseSyntaxTree(sourceBuilder.ToString(), path: "bench.cs");
        var comp = CSharpCompilation.Create(
            "Benchmark",
            new[] { tree },
            coreRefs);
        var diags = comp.GetDiagnostics();
    }
}

The scenario looks like:

#nullable enable
object? x = null;
C? c = null;
if (
    c?.M(x = 0) == (
    c?.M(x = 1) == (
    c?.M(x = 2) == (
    c?.M(x = 3) == (
    c?.M(x = 4) == (
    c?.M(x = 5) == (
    c?.M(x = 6) == (
    c?.M(x = 7) == (
    c?.M(x = 8) == (
    c?.M(x = 9) == (
    c!.M(x)
    ))))))))))
    )
{
}

class C
{
    public bool M(object? obj) => false;
}

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
AMD Ryzen 9 3900X, 1 CPU, 24 logical and 12 physical cores
.NET Core SDK=6.0.100-preview.3.21202.5
[Host] : .NET Core 5.0.4 (CoreCLR 5.0.421.11614, CoreFX 5.0.421.11614), X64 RyuJIT
.NET Core 5.0 : .NET Core 5.0.4 (CoreCLR 5.0.421.11614, CoreFX 5.0.421.11614), X64 RyuJIT

Job=.NET Core 5.0 Runtime=.NET Core 5.0

It seems to me like the level of performance we're seeing here is adequate. It certainly starts to get bad as the depth increases, but that's approaching the point where we expect only machine-generated code to reach, and machine-generated code like this should probably disable the nullable context regardless.

/cc @jcouv @AlekseyTs @333fred

@jcouv @333fred I believe I have addressed all the comments. Please take another look when you get the chance.

We have an existing benchmarks project at dotnet/performance. I don't think we should add this.

Refers to: src/Tools/CompilerBenchmarks/CompilerBenchmarks.csproj:1 in 4718b68. [](commit_id = 4718b68, deletion_comment = False)

Done review pass (commit 4)

We have an existing benchmarks project at dotnet/performance. I don't think we should add this.

Perhaps the right thing is just to remove the new project and let the test case live on in OverloadResolutionPerfTests.cs.

/azp run

@333fred @jcouv I believe I've addressed all your comments. Please let me know if you have any more.