YamlDotNet
YamlDotNet copied to clipboard
aaubry
Eliminate allocations from CharacterAnalyzer<StringLookAheadBuffer>
In the provided serialization benchmark, there's an allocation of a StringLookAheadBuffer and a CharacterAnalyzer<T> for each serialized entity:
| Type | Allocations |
|---|---|
| | - YamlDotNet.Core.StringLookAheadBuffer | 40,018 |
| | - YamlDotNet.Core.CharacterAnalyzer<YamlDotNet.Core.StringLookAheadBuffer> | 40,000 |
The CharacterAnalyzer allocations can be eliminated entirely by changing it from a class to a readonly struct.
StringLookAheadBuffer however has mutable state and thus can't become a struct without more significant refactoring. Instead, I chose to pool these objects. I tried to use the existing ConcurrentObjectPool. However, doing so showed a ~12% CPU regression on .NET Framework only (no CPU change on .NET Core). I instead updated the ConcurrentObjectPool to the newer ObjectPool that's part of Microsoft.Extensions.ObjectPool. I could bring that dependency in as a NuGet dependency, but because the package currently has no external dependencies, I chose to instead inline the implementation. I also upgraded the existing StringBuilderPool to use the new object pool as well to avoid duplicate object pool implementations. Note that the Microsoft.Extensions.ObjectPool has different defaults than ConcurrentObjectPool did for the initial and max size of StringBuilder to pool. I picked the old values to prevent a behavior change.
There's still a small CPU time regression on .NET Framework (~8%) in exchange for a comparable memory win on both .NET Framework and .NET Core. I can cut the size of the regression in half without impacting the memory savings by removing the RAII-style BufferWrapper struct and doing manual try...finally cleanup. I opt-ed not to in order to keep the coding style consistent, however I'm happy to make that change as well if desired.
Before
| Method | Job | Runtime | Mean | Error | StdDev | Gen0 | Gen1 | Allocated |
|----------- |----------------------------- |------------------- |----------:|---------:|---------:|----------:|---------:|----------:|
| Serializer | MediumRun-.NET 8.0 | .NET 8.0 | 53.00 ms | 1.580 ms | 2.316 ms | 2000.0000 | 500.0000 | 24.44 MB |
| Serializer | MediumRun-.NET Framework 4.7 | .NET Framework 4.7 | 130.08 ms | 1.907 ms | 2.795 ms | 7750.0000 | 500.0000 | 48 MB |
After
| Method | Job | Runtime | Mean | Error | StdDev | Gen0 | Gen1 | Allocated |
|----------- |----------------------------- |------------------- |----------:|---------:|---------:|----------:|---------:|----------:|
| Serializer | MediumRun-.NET 8.0 | .NET 8.0 | 49.14 ms | 1.185 ms | 1.774 ms | 2000.0000 | 500.0000 | 22.3 MB |
| Serializer | MediumRun-.NET Framework 4.7 | .NET Framework 4.7 | 139.70 ms | 2.000 ms | 2.932 ms | 7250.0000 | 500.0000 | 45.86 MB |
Hi. Thanks of taking a look at this. Should I test the most recent stable build or something else?
You need to compile the build from source if you are interested in testing these changes.
Hello. Testing on the 0.6.4.0 aux button mapping maps to the scroll wheel not the buttons on the tablet. Perhaps I should be testing another version??
Yes, considering this hasn't been merged yet the code isn't even in the official git repo yet, let alone a release.
If you want to test this you'll have to compile it.
I am willing to test but i am not in a position to compile it myself, sorry.
Rerunning CI on my fork so I don't have to push a new commit to this PR. Windows' publish should be ready in a couple minutes, if you have plugins on your current installation of OTD remove the entire %localappdata%\OpenTabletDriver folder. 0.6.x and master are not compatible.
https://github.com/jamesbt365/OpenTabletDriver/actions/runs/10251061061
still the same behaviour. Aux keys 1 & 2 not binding but rather setting them maps to the scroll wheel.
Could you go to help > export diagnostics, save and send that file here?
