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

Change Mark to be readonly struct #647

Merged
merged 1 commit into from Nov 15, 2021
Merged

Conversation

lahma
Copy link
Contributor

@lahma lahma commented Nov 13, 2021

!! This is binary breaking change !!

To reduce memory allocations and improve scanning speed (less GC) I've changed Mark to be a read-only struct. In parsing test code profiling showed that over 20% of memory allocations were from constructing Mark instances. This is a binary breaking change, but I'd argue the performance benefits are quite good. The API itself didn't change

Using in modifier passing Mark instances around as struct size exceeds pointer size.

Also introduced ThrowHelper to allow inlining Mark constructor.

Benchmark Code

Uses the large YAML from issue #519 . I noticed you don't have a benchmark project currently so I'm running my local one.

[MemoryDiagnoser]
public class YamlStreamBenchmark
{
    private string yamlString = "";

    [GlobalSetup]
    public void Setup()
    {
        yamlString = File.ReadAllText(@"c:\work\saltern.yml");
    }

    [Benchmark]
    public void Load()
    {
        var yamlStream = new YamlStream();
        yamlStream.Load(new StringReader(yamlString));
    }
}

Results

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=6.0.100
  [Host]     : .NET 6.0.0 (6.0.21.52210), X64 RyuJIT
  DefaultJob : .NET 6.0.0 (6.0.21.52210), X64 RyuJIT

YamlStreamBenchmark

Diff Method Mean Error Gen 0 Gen 1 Gen 2 Allocated
Old Load 96.71 ms 1.551 ms 5833.3333 2833.3333 1000.0000 81 MB
New 73.13 ms (-24%) 1.319 ms 4428.5714 (-24%) 2000.0000 (-29%) 714.2857 (-29%) 63 MB (-22%)

@aaubry
Copy link
Owner

aaubry commented Nov 15, 2021

I noticed you don't have a benchmark project currently so I'm running my local one.

I used to have one to compare the performance across releases, but it was difficult to maintain and provided little value so it was removed. Feel free to add a benchmark if you feel it would be useful.

@lahma
Copy link
Contributor Author

lahma commented Nov 15, 2021

@aaubry would you like it as part of this PR or separate?

@aaubry
Copy link
Owner

aaubry commented Nov 15, 2021

@aaubry would you like it as part of this PR or separate?

As you prefer. I was about to merge this, but I can wait.

@lahma
Copy link
Contributor Author

lahma commented Nov 15, 2021

Please merge, I'll create a separate one! 😄

@aaubry aaubry merged commit f9a28fc into aaubry:master Nov 15, 2021
@lahma lahma deleted the struct-mark branch November 15, 2021 10:24
@aaubry
Copy link
Owner

aaubry commented Jul 23, 2022

This feature has been released in version 12.0.0.

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.

None yet

2 participants