Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use compiler-generated delegate caches in ToDelimitedString #966

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

atifaziz
Copy link
Member

This PR removes all StringBuilderAppenders members that were designed to serve as static caches of delegates for use in ToDelimitedString implementations. This may have been significant in the past, but now using a simple static lambda seems to be simpler and the compiler-generated cached delegate run faster. It does however come at the cost of an additional allocation because while the lambdas themselves are static, the delegate caching gets hosted into a non-static class (<>c below) generated by the C# compiler and the bodies converted into regular instance-based methods:

[Serializable]
[CompilerGenerated]
private sealed class <>c
{
    [System.Runtime.CompilerServices.Nullable(0)]
    public static readonly <>c <>9 = new <>c();

    // ...omitted for brevity...

    public static Func<StringBuilder, bool, StringBuilder> <>9__281_0;
    public static Func<StringBuilder, byte, StringBuilder> <>9__282_0;
    public static Func<StringBuilder, char, StringBuilder> <>9__283_0;
    public static Func<StringBuilder, decimal, StringBuilder> <>9__284_0;
    public static Func<StringBuilder, double, StringBuilder> <>9__285_0;
    public static Func<StringBuilder, float, StringBuilder> <>9__286_0;
    public static Func<StringBuilder, int, StringBuilder> <>9__287_0;
    public static Func<StringBuilder, long, StringBuilder> <>9__288_0;
    public static Func<StringBuilder, sbyte, StringBuilder> <>9__289_0;
    public static Func<StringBuilder, short, StringBuilder> <>9__290_0;
    public static Func<StringBuilder, string, StringBuilder> <>9__291_0;
    public static Func<StringBuilder, uint, StringBuilder> <>9__292_0;
    public static Func<StringBuilder, ulong, StringBuilder> <>9__293_0;
    public static Func<StringBuilder, ushort, StringBuilder> <>9__294_0;

    // ...omitted for brevity...

    internal StringBuilder <ToDelimitedString>b__281_0(StringBuilder sb, bool e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__282_0(StringBuilder sb, byte e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__283_0(StringBuilder sb, char e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__284_0(StringBuilder sb, decimal e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__285_0(StringBuilder sb, double e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__286_0(StringBuilder sb, float e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__287_0(StringBuilder sb, int e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__288_0(StringBuilder sb, long e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__289_0(StringBuilder sb, sbyte e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__290_0(StringBuilder sb, short e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__291_0(StringBuilder sb, string e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__292_0(StringBuilder sb, uint e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__293_0(StringBuilder sb, ulong e) => sb.Append(e);
    internal StringBuilder <ToDelimitedString>b__294_0(StringBuilder sb, ushort e) => sb.Append(e);
}

Per dotnet/roslyn#51344, this is done so because:

…the runtime is actually faster here in the non-static case than in the static case.

In the StackOverflow question “Delegate caching behavior changes in Roslyn”, @Pilchie also notes in a comment:

The reason it's faster is because delegate invokes are optimized for instance methods and have space on the stack for them. To call a static method they have to shift parameters around.

Benchmarks seem to agree:

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.1265)
Intel Core i7-1065G7 CPU 1.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.103
  [Host] : .NET 7.0.3 (7.0.323.6910), X86 RyuJIT AVX2
Method Mean Error StdDev Allocated
StaticLambda 2.138 ns 0.0208 ns 0.0185 ns -
StaticMethodGroup 3.003 ns 0.0296 ns 0.0276 ns -
StaticLocalFunction 3.026 ns 0.0607 ns 0.0567 ns -
CustomCachedDelegate 2.447 ns 0.0383 ns 0.0340 ns -

@codecov
Copy link

codecov bot commented Feb 18, 2023

Codecov Report

Merging #966 (2ffae2f) into master (4a9041d) will decrease coverage by 0.04%.
The diff coverage is 7.14%.

❗ Current head 2ffae2f differs from pull request most recent head d9605ed. Consider uploading reports for the commit d9605ed to get more accurate results

@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
- Coverage   92.52%   92.49%   -0.04%     
==========================================
  Files         113      113              
  Lines        3440     3426      -14     
  Branches     1024     1024              
==========================================
- Hits         3183     3169      -14     
  Misses        192      192              
  Partials       65       65              
Impacted Files Coverage Δ
MoreLinq/ToDelimitedString.g.cs 62.85% <7.14%> (-6.20%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@atifaziz atifaziz merged commit d2217ce into morelinq:master Feb 18, 2023
@atifaziz atifaziz deleted the x-StringBuilderAppenders branch February 18, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants