Allocation-free IMemoryGroup<T> enumeration #1173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1173 +/- ##
==========================================
- Coverage 82.47% 82.47% -0.01%
==========================================
Files 687 688 +1
Lines 29871 29892 +21
Branches 3378 3378
==========================================
+ Hits 24637 24654 +17
- Misses 4534 4538 +4
Partials 700 700
Continue to review full report at Codecov.
|
|
@Sergio0694 Looks good to me, I'd like @antonfirsov to review this though to be sure. |
|
@JimBobSquarePants No worries (there's no rush), glad you like it though! 😄 Also yeah absolutely, in fact I had already signed up @antonfirsov for a review, felt appropriate considering he was the original author of the memory group types, plus this PR is small but arguably a bit weird, so having more people double checking it doesn't hurt! 👍 |
|
Ok, will do it in the weekend. |
| /// </summary> | ||
| /// <typeparam name="T">The element type.</typeparam> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public ref struct MemoryGroupEnumerator<T> |
There was a problem hiding this comment.
Technically, this doesn't require to be a ref struct (all members are heapable). Have you added the constraint to ensure it's only used in method locals?
There was a problem hiding this comment.
Yeah, it was mostly a precaution to prevent users from doing stuff like storing the iterator as a field in a class for later use, or other weird things. This type should really only ever be used implicitly by the compiler when using foreach, it should be completely transparent to users. Same reason for that EditorBrowsableState.Never as well 😄
| public override int Count => this.source.Length; | ||
| public override int Count | ||
| { | ||
| [MethodImpl(InliningOptions.ShortMethod)] |
There was a problem hiding this comment.
The type is not sealed. Will Count be ever inlined actually?
There was a problem hiding this comment.
I've added that because those methods are being called directly from the enumerator:
In theory (I hope), the JIT should be able to devirtualize and then inline that call, since all APIs are internal, so I assume it could potentially know the type is definitely just the one declared from there as a parameter. Of course, we could also declare those types as sealed, to be honest I'm not even sure why they're not already? 🤔
There was a problem hiding this comment.
I'm unsure about JIT here, anyways this is a question of low importance.
There was a problem hiding this comment.
Better one AggressiveInlining in excess than lacking one 🤣
|
Yay for even less allocations! 🎉🚀 @JimBobSquarePants We might also want to follow up on my comment here and consider making those sub-types |
|
@Sergio0694 Yeah, as long as there's no reason we can think of that we would inherit from them @antonfirsov ? |
Prerequisites
Description
This PR introduces some speed/memory improvements to the
IMemoryGroup<T>interface and the various classes implementing it. In particular, I've tweaked a bit the existing enumerators, and I've added a new value-typeMemoryGroupEnumerator<T>type that can be used through the C# 8 pattern-based interfaces forref structfeature (see here). This change makes it so that whenever you useforeachon anIMemoryGroup<T>instance, the compiler will pick the new method over theIEnumerable<Memory<T>>.GetEnumerator()one, which lets us have exactly the same functionality as before (no changes at all needed in the rest of the codebase), but faster and without memory allocations like before. Pinging both James and Anton for review as this PR involves both the API surface as well as the actual performance of theIMemoryGroup<T>types. 😄🚀