unoplatform / uno

Open-source platform for building cross-platform native Mobile, Web, Desktop and Embedded apps quickly. Create rich, C#/XAML, single-codebase apps from any IDE. Hot Reload included! 90m+ NuGet Downloads!!
https://platform.uno
Apache License 2.0
8.78k stars 707 forks source link

Reduce allocations in `XamlMember.ToString` #13232

Open Youssef1313 opened 1 year ago

Youssef1313 commented 1 year ago

image

image

KevinFerati commented 3 months ago

Before diving in a PR, I'll put my analysis here.

The issue arises with String.Concat(params string[]) in the else branch, which allocates two string[] (one when passing the arguments, the other one in the function itself). I've played around with some solutions : using a StringBuilder with a predefined capacity, using a stack and heap-allocated char buffer (which are then filled with the strings by copying them character by character).

Here's a (quick and ugly) benchmark comparing them following the XamlMember.ToString semantic : image

Here's one with bigger strings for good measure : image

The stack-allocated version has 72B (~1/3 to 15%) less allocations, but takes more time, probably due to the manual copying from the source strings. Would that be an issue ?†.

I can gladly give the source for my benchmarks and the strings i've used if anyone needs it :)

† EDIT : I created a new version where I use a pointer to a string and write directly in it. This seems to be more in line with what Concat gives, although I can't explain why.

agneszitte commented 3 months ago

Before diving in a PR, I'll put my analysis here.

thanks a lot for the details @KevinFerati!

fyi @jeromelaban, @Youssef1313, @ebariche

KevinFerati commented 3 months ago

Before diving in a PR, I'll put my analysis here.

thanks a lot for the details @KevinFerati!

fyi @jeromelaban, @Youssef1313, @ebariche

De rien ! Also, to add to my initial post : I've tried benchmarking the difference in the number of allocated bytes between a litteral string and one created by a stack-allocated char buffer or by mutating directly a newly created string. Turns out out that they have the same number of allocated bytes, so I don't think we can go below that threshold.

That being said ; is XamlMember.ToString() frequently used for the same instance ? If that's the case, we could store the string representation of the said instance and just return it in the subsequent calls of ToString. (Sorry, i'm very new to Uno 😄)

jeromelaban commented 3 months ago

Those are interesting findings, thanks.

Looking at the code further XamlMember.ToString() should not necessarily be used at all in BuildExtendedProperties, as it is not supposed represent a value that can be used in the generated code (other than for diagnostics). I wonder if we should be removing its use altogether. Finding its actual uses would be better than fixing the allocation problem.

KevinFerati commented 3 months ago

XamlMember.ToString() is used by its GetHashCode() which, according to #13228 , is in turn called by the underlying Dictionary when it's memoized. Another Dictionary with a key of type XamlMember can be found in ParsedMarkupExtensionInfo (though it seems like a lot of its keys could be XamlDirectives, but i'm not quite sure).

Anyway, basically everything that uses the hash code of a XamlMember.

jeromelaban commented 3 months ago

Right. There are multiple ways to think about this:

  1. We could cache ToString(), though it would still be generating strings
  2. We could modify GetHashCode to create a hash based on the presence of a DeclaringType without creating strings, then cache the hashcode value as XamlMember seems to effectively be immutable.
  3. Change the XamlMember redirection type to do something similar as point 2.

I'd likely prefer solution 2.