Avalonia
Avalonia copied to clipboard
feat: StringBuilderCache
What does the pull request do?
Avoid multiple allocation of StringBuilder
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
- [ ] Added unit tests (if possible)?
- [ ] Added XML documentation to any related classes?
- [ ] Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation
Breaking changes
Obsoletions / Deprecations
Fixed issues
Do you have benchmarks? ~~Also isn't thread-safety a concern as well? This would prevent us from using something like TraceLogSink
etc. from different threads.~~
NVM it's thread local
I don't have any benchmarks at the moment. I am trying to make it one that is significant for avalonia. Do you have any suggestions?
The only changed hot path would be logging
I did not understand what I have to do. Should I close the PR?
You can test this PR using the following package version. 0.10.999-cibuild0021777-beta
. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]
I did not understand what I have to do. Should I close the PR?
I think @kekekeks is saying that the only performance-related hot path to really consider with this change is logging. That may indicate benchmarks only really need to focus on that -- if any are required. Hopefully he clarifies though.
I understood the same way as @robloo
I was answering the benchmarking question.
The PR itself is beneficial anyway since it removes extra allocations.
BTW I am fine with the PR as well, was just wondering :) Generally, it's good to undermine performance PRs with benchmarks to make sure they don't accidentally regress stuff.
@maxkatz6 @kekekeks Can this be merged as-is? I can't see anything wrong with it and I'm not sure benchmarks are really required. The actual StringBuilderCache is taken from upstream .net runtime repo -- if Microsoft is OK with it, it should be good here too.
You can test this PR using the following package version. 0.10.999-cibuild0022675-beta
. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]
BTW, since our strings are relatively small, we can just use ValueStringBuilder with a stack-allocated initial buffer like .NET runtime does
You can test this PR using the following package version. 11.0.999-cibuild0023896-beta
. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]