search

danipen / TextMateSharp

A port of tm4e to bring TextMate grammars to dotnet ecosystem
MIT License
87 stars 15 forks source link

Does PList need to keep creating new StringBuilders ? #45

Closed Numpsy closed 1 year ago

Numpsy commented 1 year ago

An observation / question whilst liiking at using CopyString intsead of GetString in JSONPListParser.

The reason I ask is that I made a small benchmark.net project that just loads the C# grammer and the dark theme, and with the current code (the System.Text.Json version), i get this:

BenchmarkDotNet=v0.13.5, OS=Windows 10 (10.0.19045.2728/22H2/2022Update), VM=Hyper-V
11th Gen Intel Core i7-1185G7 3.00GHz, 1 CPU, 4 logical and 2 physical cores
.NET SDK=7.0.202
  [Host]               : .NET 7.0.4 (7.0.423.11508), X64 RyuJIT AVX2
  .NET 6.0             : .NET 6.0.15 (6.0.1523.11507), X64 RyuJIT AVX2
  .NET 7.0             : .NET 7.0.4 (7.0.423.11508), X64 RyuJIT AVX2
  .NET Framework 4.7.2 : .NET Framework 4.8 (4.8.4614.0), X64 RyuJIT VectorSize=256

|            Method |                  Job |              Runtime |        Mean |     Error |    StdDev |     Gen0 |     Gen1 |  Allocated |
|------------------ |--------------------- |--------------------- |------------:|----------:|----------:|---------:|---------:|-----------:|
| LoadDarkPlusTheme |             .NET 6.0 |             .NET 6.0 |    32.65 us |  0.563 us |  0.603 us |  11.4746 |   0.9155 |   70.58 KB |
|     LoadCSGrammar |             .NET 6.0 |             .NET 6.0 | 1,077.55 us |  5.228 us |  4.891 us | 279.2969 | 121.0938 | 1719.05 KB |
| LoadDarkPlusTheme |             .NET 7.0 |             .NET 7.0 |    27.89 us |  0.439 us |  0.389 us |  11.5051 |   0.9155 |   70.58 KB |
|     LoadCSGrammar |             .NET 7.0 |             .NET 7.0 | 1,008.97 us | 12.943 us | 10.105 us | 279.2969 | 121.0938 | 1719.05 KB |
| LoadDarkPlusTheme | .NET Framework 4.7.2 | .NET Framework 4.7.2 |    53.95 us |  0.197 us |  0.164 us |  11.7798 |   0.9155 |   72.43 KB |
|     LoadCSGrammar | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1,652.90 us | 16.280 us | 15.228 us | 287.1094 | 134.7656 | 1768.32 KB |

but with this change I get:

|            Method |                  Job |              Runtime |        Mean |     Error |    StdDev |      Median |     Gen0 |    Gen1 |  Allocated |
|------------------ |--------------------- |--------------------- |------------:|----------:|----------:|------------:|---------:|--------:|-----------:|
| LoadDarkPlusTheme |             .NET 6.0 |             .NET 6.0 |    27.84 us |  0.550 us |  0.514 us |    27.55 us |   6.5613 |  0.4883 |    40.3 KB |
|     LoadCSGrammar |             .NET 6.0 |             .NET 6.0 |   954.05 us | 18.628 us | 27.881 us |   951.18 us | 167.9688 | 72.2656 | 1031.75 KB |
| LoadDarkPlusTheme |             .NET 7.0 |             .NET 7.0 |    23.26 us |  0.459 us |  0.948 us |    22.86 us |   6.5613 |  0.4883 |    40.3 KB |
|     LoadCSGrammar |             .NET 7.0 |             .NET 7.0 |   867.26 us | 17.322 us | 38.743 us |   847.65 us | 167.9688 | 94.7266 | 1031.75 KB |
| LoadDarkPlusTheme | .NET Framework 4.7.2 | .NET Framework 4.7.2 |    48.68 us |  0.407 us |  0.340 us |    48.77 us |   6.7749 |  0.4883 |   41.79 KB |
|     LoadCSGrammar | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1,608.01 us | 31.973 us | 66.739 us | 1,585.66 us | 173.8281 | 66.4063 | 1079.56 KB |

The timings are a bit variable running the test on my laptop, but the reported reduction in allocations was much larger that I expected (to the point I wondered if something else is going on, but the unit tests all pass so).

Anyway, I'll leave it here for comments.

danipen commented 1 year ago

Yes allocation are a little bit better and the change look safe. Let's merge it.