Avalonia icon indicating copy to clipboard operation
Avalonia copied to clipboard

Published 20 hours ago • verified publisher icon AvaloniaUI

feat: StringBuilderCache

Open workgroupengineering opened this issue 2 years ago • 12 comments

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

workgroupengineering avatar Jul 07 '22 08:07 workgroupengineering

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

pr8x avatar Jul 07 '22 09:07 pr8x

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?

workgroupengineering avatar Jul 07 '22 13:07 workgroupengineering

The only changed hot path would be logging

kekekeks avatar Jul 08 '22 07:07 kekekeks

I did not understand what I have to do. Should I close the PR?

workgroupengineering avatar Jul 09 '22 08:07 workgroupengineering

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]

avaloniaui-team avatar Jul 09 '22 09:07 avaloniaui-team

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.

robloo avatar Jul 11 '22 02:07 robloo

I understood the same way as @robloo

timunie avatar Jul 11 '22 04:07 timunie

I was answering the benchmarking question.

The PR itself is beneficial anyway since it removes extra allocations.

kekekeks avatar Jul 11 '22 08:07 kekekeks

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.

pr8x avatar Jul 14 '22 11:07 pr8x

@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.

robloo avatar Jul 28 '22 02:07 robloo

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]

avaloniaui-team avatar Aug 01 '22 08:08 avaloniaui-team

BTW, since our strings are relatively small, we can just use ValueStringBuilder with a stack-allocated initial buffer like .NET runtime does

kekekeks avatar Aug 01 '22 09:08 kekekeks

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]

avaloniaui-team avatar Sep 17 '22 08:09 avaloniaui-team