Skip to content

Mark record struct generated methods as readonly where possible#55039

Merged
RikkiGibson merged 26 commits intodotnet:mainfrom
AndreyTretyak:dev/andreyt/54419-record-struct-readonly-methods
Aug 10, 2021
Merged

Mark record struct generated methods as readonly where possible#55039
RikkiGibson merged 26 commits intodotnet:mainfrom
AndreyTretyak:dev/andreyt/54419-record-struct-readonly-methods

Conversation

@AndreyTretyak
Copy link
Contributor

@AndreyTretyak AndreyTretyak commented Jul 22, 2021

Fixes #54419

@ghost ghost added the Area-Compilers label Jul 22, 2021
@AndreyTretyak AndreyTretyak marked this pull request as ready for review July 23, 2021 02:32
@AndreyTretyak AndreyTretyak requested review from a team as code owners July 23, 2021 02:32
AndreyTretyak and others added 3 commits July 23, 2021 19:20
…inerSymbol.cs

Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
@RikkiGibson
Copy link
Member

RikkiGibson commented Jul 23, 2021

I also wanted to note that I benchmarked ToString performance when the method was readonly vs non-readonly. The first table is from a simpler non-generic scenario. The second scenario tries to account for the impact of copying generic fields constrained to struct within the ToString before calling the field's ToString.

(edit: I realized it would be simpler and result in a more apples-to-apples comparison to simply make some of the record struct declarations 'readonly' so that the synthesized ToString would be effectively readonly. Now the difference appears to be within margin of error, but still good to see the numbers on it.)

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.19042.1110 (20H2/October2020Update)
AMD Ryzen 9 3900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=6.0.100-preview.6.21355.2
  [Host]     : .NET 6.0.0 (6.0.21.35212), X64 RyuJIT
  DefaultJob : .NET 6.0.0 (6.0.21.35212), X64 RyuJIT

Non-generic

Method Mean Error StdDev
ToString 334.2 ns 0.92 ns 0.86 ns
ToStringReadonly 333.4 ns 3.35 ns 3.14 ns
ToString_RefReadonly 352.3 ns 5.85 ns 5.47 ns
ToStringReadonly_RefReadonly 346.0 ns 4.11 ns 3.85 ns

Generic where T : struct

Method Mean Error StdDev
ToString 327.2 ns 2.55 ns 2.13 ns
ToStringReadonly 333.9 ns 5.09 ns 4.52 ns
ToString_RefReadonly 353.6 ns 3.71 ns 3.47 ns
ToStringReadonly_RefReadonly 351.9 ns 2.34 ns 2.19 ns
Benchmark source
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Runtime.CompilerServices;
using System.Text;

namespace bench_readonly
{
    class Program
    {
        static void Main(string[] args)
        {
            var summary = BenchmarkRunner.Run(typeof(Program).Assembly);
        }
    }

    [MarkdownExporterAttribute.GitHub]
    public class ToStringFix
    {
        [Benchmark]
        public new void ToString()
        {
            var rec1 = new Rec1("abc", false, 42, new Rec2("def", 123));
            rec1.ToString();
        }

        [Benchmark]
        public void ToStringReadonly()
        {
            var rec3 = new Rec3("abc", false, 42, new Rec4("def", 123));
            rec3.ToString();
        }

        [Benchmark]
        public void ToString_RefReadonly()
        {
            var rec1 = new Rec1("abc", false, 42, new Rec2("def", 123));
            local(in rec1);

            void local(in Rec1 rec1) => rec1.ToString();
        }

        [Benchmark]
        public void ToStringReadonly_RefReadonly()
        {
            var rec3 = new Rec3("abc", false, 42, new Rec4("def", 123));
            local(in rec3);

            void local(in Rec3 rec3) => rec3.ToString();
        }

        public record struct Rec1(string Prop1, bool Prop2, int Prop3, Rec2 Prop4);
        public record struct Rec2(string Prop1, int Prop2);

        public readonly record struct Rec3(string Prop1, bool Prop2, int Prop3, Rec4 Prop4);
        public readonly record struct Rec4(string Prop1, int Prop2);
    }

    [MarkdownExporterAttribute.GitHub]
    public class ToStringFix_StructConstrainedField
    {
        [Benchmark]
        public new void ToString()
        {
            var rec1 = new Rec1<Rec2>("abc", false, 42, new Rec2("def", 123));
            rec1.ToString();
        }

        [Benchmark]
        public void ToStringReadonly()
        {
            var rec3 = new Rec3<Rec4>("abc", false, 42, new Rec4("def", 123));
            rec3.ToString();
        }

        [Benchmark]
        public void ToString_RefReadonly()
        {
            var rec1 = new Rec1<Rec2>("abc", false, 42, new Rec2("def", 123));
            local(in rec1);

            void local(in Rec1<Rec2> rec1) => rec1.ToString();
        }

        [Benchmark]
        public void ToStringReadonly_RefReadonly()
        {
            var rec3 = new Rec3<Rec4>("abc", false, 42, new Rec4("def", 123));
            local(in rec3);

            void local(in Rec3<Rec4> rec3) => rec3.ToString();
        }

        public record struct Rec1<T>(string Prop1, bool Prop2, int Prop3, T Prop4) where T : struct;

        public record struct Rec2(string Prop1, int Prop2);

        public readonly record struct Rec3<T>(string Prop1, bool Prop2, int Prop3, T Prop4) where T : struct;

        public readonly record struct Rec4(string Prop1, int Prop2);
    }
}

@AndreyTretyak
Copy link
Contributor Author

@RikkiGibson It looks to me that pipeline failure is unrelated to this change. Is it possible and if so what would be recommended actions? Wait for a fix in main and merge changes into PR?

@RikkiGibson
Copy link
Member

Let's just try re-running once more, if they still fail, then let's look at merging 'main' into the PR branch.

@RikkiGibson
Copy link
Member

@jcouv @dotnet/roslyn-compiler for second review.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 26)

@RikkiGibson
Copy link
Member

Thanks for the contribution @AndreyTretyak!

@RikkiGibson RikkiGibson merged commit ba75cb8 into dotnet:main Aug 10, 2021
@ghost ghost added this to the Next milestone Aug 10, 2021
@AndreyTretyak AndreyTretyak deleted the dev/andreyt/54419-record-struct-readonly-methods branch August 14, 2021 12:34
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Record structs: emit readonly modifier for synthesized members

4 participants