-
-
Notifications
You must be signed in to change notification settings - Fork 891
Allocation-free IMemoryGroup<T> enumeration #1173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
404c086
f1f3cb7
c2ec5c4
a7d6acf
aae14cf
989300e
ad69c23
c237de5
9823eed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| // Copyright (c) Six Labors and contributors. | ||
| // Licensed under the Apache License, Version 2.0. | ||
|
|
||
| using System; | ||
| using System.ComponentModel; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace SixLabors.ImageSharp.Memory | ||
| { | ||
| /// <summary> | ||
| /// A value-type enumerator for <see cref="MemoryGroup{T}"/> instances. | ||
| /// </summary> | ||
| /// <typeparam name="T">The element type.</typeparam> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public ref struct MemoryGroupEnumerator<T> | ||
| where T : struct | ||
| { | ||
| private readonly IMemoryGroup<T> memoryGroup; | ||
| private readonly int count; | ||
| private int index; | ||
|
|
||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| internal MemoryGroupEnumerator(MemoryGroup<T>.Owned memoryGroup) | ||
| { | ||
| this.memoryGroup = memoryGroup; | ||
| this.count = memoryGroup.Count; | ||
| this.index = -1; | ||
| } | ||
|
|
||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| internal MemoryGroupEnumerator(MemoryGroup<T>.Consumed memoryGroup) | ||
| { | ||
| this.memoryGroup = memoryGroup; | ||
| this.count = memoryGroup.Count; | ||
| this.index = -1; | ||
| } | ||
|
|
||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| internal MemoryGroupEnumerator(MemoryGroupView<T> memoryGroup) | ||
| { | ||
| this.memoryGroup = memoryGroup; | ||
| this.count = memoryGroup.Count; | ||
| this.index = -1; | ||
| } | ||
|
|
||
| /// <inheritdoc cref="System.Collections.Generic.IEnumerator{T}.Current"/> | ||
| public Memory<T> Current | ||
| { | ||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| get => this.memoryGroup[this.index]; | ||
| } | ||
|
|
||
| /// <inheritdoc cref="System.Collections.IEnumerator.MoveNext"/> | ||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| public bool MoveNext() | ||
| { | ||
| int index = this.index + 1; | ||
|
|
||
| if (index < this.count) | ||
| { | ||
| this.index = index; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -2,16 +2,17 @@ | |||
| // Licensed under the Apache License, Version 2.0. | ||||
|
|
||||
| using System; | ||||
| using System.Buffers; | ||||
| using System.Collections.Generic; | ||||
| using System.Linq; | ||||
| using System.Runtime.CompilerServices; | ||||
|
|
||||
| namespace SixLabors.ImageSharp.Memory | ||||
| { | ||||
| internal abstract partial class MemoryGroup<T> | ||||
| { | ||||
| // Analogous to the "consumed" variant of MemorySource | ||||
| private sealed class Consumed : MemoryGroup<T> | ||||
| /// <summary> | ||||
| /// A <see cref="MemoryGroup{T}"/> implementation that consumes the underlying memory buffers. | ||||
| /// </summary> | ||||
| public sealed class Consumed : MemoryGroup<T>, IEnumerable<Memory<T>> | ||||
| { | ||||
| private readonly Memory<T>[] source; | ||||
|
|
||||
|
|
@@ -22,16 +23,31 @@ public Consumed(Memory<T>[] source, int bufferLength, long totalLength) | |||
| this.View = new MemoryGroupView<T>(this); | ||||
| } | ||||
|
|
||||
| public override int Count => this.source.Length; | ||||
| public override int Count | ||||
| { | ||||
| [MethodImpl(InliningOptions.ShortMethod)] | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type is not sealed. Will
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added that because those methods are being called directly from the enumerator: ImageSharp/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroupEnumerator{T}.cs Line 26 in c237de5
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure about JIT here, anyways this is a question of low importance.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better one |
||||
| get => this.source.Length; | ||||
| } | ||||
|
|
||||
| public override Memory<T> this[int index] => this.source[index]; | ||||
|
|
||||
| public override IEnumerator<Memory<T>> GetEnumerator() | ||||
| /// <inheritdoc/> | ||||
| [MethodImpl(InliningOptions.ShortMethod)] | ||||
| public override MemoryGroupEnumerator<T> GetEnumerator() | ||||
| { | ||||
| return new MemoryGroupEnumerator<T>(this); | ||||
| } | ||||
|
|
||||
| /// <inheritdoc/> | ||||
| IEnumerator<Memory<T>> IEnumerable<Memory<T>>.GetEnumerator() | ||||
| { | ||||
| for (int i = 0; i < this.source.Length; i++) | ||||
| { | ||||
| yield return this.source[i]; | ||||
| } | ||||
| /* The runtime sees the Array class as if it implemented the | ||||
| * type-generic collection interfaces explicitly, so here we | ||||
| * can just cast the source array to IList<Memory<T>> (or to | ||||
| * an equivalent type), and invoke the generic GetEnumerator | ||||
| * method directly from that interface reference. This saves | ||||
| * having to create our own iterator block here. */ | ||||
| return ((IList<Memory<T>>)this.source).GetEnumerator(); | ||||
| } | ||||
|
|
||||
| public override void Dispose() | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatEditorBrowsableState.Neveras well 😄