Improve performance of JoinParts for spans
#42
Conversation
Sewer56
added
the
meta-performance
erri120
changed the title
JoinParts for spans
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #42 +/- ##
==========================================
+ Coverage 87.19% 87.20% +0.01%
==========================================
Files 42 42
Lines 3357 3362 +5
Branches 546 544 -2
==========================================
+ Hits 2927 2932 +5
Misses 371 371
Partials 59 59
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This needs some benchmarks to compare old vs. new. The span-version of JoinParts is also not used by anything except the tests, so even if the new version is more performant than the old version, we won't see that benefit.
If I remember correctly, we have a span-version and a string-version of JoinParts because they have different performance characteristics. The string-version uses string.Create to get a Span<char> while the span-version used to allocate an array "the normal way". Due to ref-struct weirdness, I couldn't get the span-version to use string.Create. This PR circumvents the ref-struct weirdness by deconstructing and reconstructing the span inside the string.Create part.
With this PR, the span- and string-versions should have similar performance characteristics (needs to be benchmarked), so the string-version should be removed.
You should also add a short comment on the unsafe part, explaining what I mentioned above about the span deconstructing and reconstructing inside the string.Create part.
Sure you can have a benchmark if you want. As a point of reference, a pin on a string (Span inclusive) is around 5 x86 instructions. No memory alloc and copy in the world would come close. |
|
The benchmark doesn't compare the old span-version to the new span-version. It compares the old span-version to the unchanged string-version. |
|
Ah, apologies. Am sleepy and haven't even noticed, one moment. |
This is the results comparing the old span-version to the new span-version. |
I forgot to account for the fact that nested tuples with deconstruction account for some very unoptimal codegen. I used a trick I've learned from one of the runtime folks, and left a small note with it. And set benchmarks to run on .NET 8, kinda overdue. |
| unsafe struct JoinPartsParams | ||
| { | ||
| internal ReadOnlySpan<char>* Left; | ||
| internal ReadOnlySpan<char>* Right; | ||
| internal IOSInformation Os; | ||
| } |
There was a problem hiding this comment.
The struct should be marked as private as well as readonly.
There was a problem hiding this comment.
I'll do it when I get back to my machine, dw
|
The current string-version is still faster than the new span-version, and all of our code uses the string-version. We can probably just remove the span-version of |
They're the same code, only catch is there's an additional 3 stack copies and 2 derererences in the Span version, which accounts for a flat nanosecond. They're unfortunately unavoidable, for now. If they use a throwhelper inside I know this is mostly dead code, I just had some spare time to kill, so I figured 'one day I'll use this, I might aswell'. An example use case that comes to mind is a future where we might want to return the parent as a |
This improves the performance of the
JoinPartsmethod, by avoiding an unnecessary heap allocation which makes another copy of the string.We pin the Span (in the case it originated from the heap), and use the pointers to the characters as input to
string.Create.