danipen / TextMateSharp

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

[experiment] Change JSonPListParser to use CopyString instead of GetString #46

Closed Numpsy closed 1 year ago

Numpsy commented 1 year ago

As suggested elsewhere, an attempt at using CopyString with a reused buffer rather than using GetString (which creates a new temporary string each time) - the idea being that the strings are added to a StringBuilder and then discarded anyway.

It's just a draft, as I don't know what the array management strategy should be (I gave it a quick try at using ArrayPool just inside AddElement and got slightly lower memory usage but also slightly slower performance, so there is probably room to tweak things more).

Anyway - with the current code I get this when testing the dark theme, C# grammer and TS grammer (I added the TS grammer as a test because there are strings in it over 7000 chars long, so reading it needs larger buffers).

|            Method |                  Job |              Runtime |        Mean |     Error |    StdDev |      Median |     Gen0 |     Gen1 |  Allocated |
|------------------ |--------------------- |--------------------- |------------:|----------:|----------:|------------:|---------:|---------:|-----------:|
| LoadDarkPlusTheme |             .NET 7.0 |             .NET 7.0 |    24.59 us |  0.476 us |  1.025 us |    24.14 us |   6.5613 |   0.4883 |    40.3 KB |
|     LoadCSGrammar |             .NET 7.0 |             .NET 7.0 |   833.95 us |  8.328 us |  7.790 us |   834.40 us | 167.9688 |  94.7266 | 1031.75 KB |
|     LoadTSGrammar |             .NET 7.0 |             .NET 7.0 | 1,376.65 us | 27.312 us | 45.633 us | 1,357.99 us | 261.7188 | 166.0156 | 1605.49 KB |
| LoadDarkPlusTheme | .NET Framework 4.7.2 | .NET Framework 4.7.2 |    46.21 us |  0.390 us |  0.346 us |    46.22 us |   6.7749 |   0.4883 |   41.79 KB |
|     LoadCSGrammar | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1,450.79 us | 11.492 us | 10.187 us | 1,451.75 us | 173.8281 |  66.4063 | 1079.56 KB |
|     LoadTSGrammar | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 2,361.21 us | 46.138 us | 43.157 us | 2,365.33 us | 269.5313 | 125.0000 | 1679.06 KB |

And with this change, I get

|------------------ |--------------------- |--------------------- |------------:|----------:|----------:|---------:|---------:|-----------:|
| LoadDarkPlusTheme |             .NET 7.0 |             .NET 7.0 |    23.14 us |  0.178 us |  0.158 us |   5.0049 |   0.3662 |   30.74 KB |
|     LoadCSGrammar |             .NET 7.0 |             .NET 7.0 |   773.25 us |  7.007 us |  6.555 us | 133.7891 |  70.3125 |  819.68 KB |
|     LoadTSGrammar |             .NET 7.0 |             .NET 7.0 | 1,268.76 us | 24.427 us | 22.849 us | 201.1719 | 138.6719 | 1238.39 KB |
| LoadDarkPlusTheme | .NET Framework 4.7.2 | .NET Framework 4.7.2 |    50.78 us |  0.460 us |  0.384 us |   5.0659 |   0.3662 |   31.48 KB |
|     LoadCSGrammar | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1,513.82 us | 22.176 us | 20.744 us | 136.7188 |  46.8750 |  848.81 KB |
|     LoadTSGrammar | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 2,373.23 us | 31.204 us | 27.662 us | 207.0313 |  93.7500 | 1287.02 KB |
danipen commented 1 year ago

@Numpsy thanks a lot for trying out this kind of stuff.

In this case, I don't see the benefits of improving this part. We're just boosting only a few us in perf and only a few KB in allocations, so I think it's not worth it if we put the code readability in the equation.

Anyway, I want to say thanks for taking a look into it and for your help.

Numpsy commented 1 year ago

I don't see the benefits of improving this part.

Yeah, one of those things where you can't really tell how much effect it'll have without making the change and measuring it, and the tiny perf change might just be a side effect of less GC allocations. I'll shelve this one for now.