Mark record struct generated methods as readonly where possible#55039
Conversation
…-record-struct-readonly-methods
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordDeconstruct.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordDeconstruct.cs
Outdated
Show resolved
Hide resolved
…hesizedRecordDeconstruct.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
…tps://github.com/AndreyTretyak/roslyn into dev/andreyt/54419-record-struct-readonly-methods
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordBaseEquals.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordClone.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordDeconstruct.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbolBase.cs
Show resolved
Hide resolved
…nly for record class
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Show resolved
Hide resolved
…inerSymbol.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
|
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 (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 RyuJITNon-generic
Generic
|
| 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);
}
}|
@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 |
|
Let's just try re-running once more, if they still fail, then let's look at merging 'main' into the PR branch. |
…-record-struct-readonly-methods
|
@jcouv @dotnet/roslyn-compiler for second review. |
src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbolBase.cs
Show resolved
Hide resolved
|
Thanks for the contribution @AndreyTretyak! |
Fixes #54419