-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Enumerable.ToArray performance improvement using InlineArray #90459
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
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 | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,7 +2,7 @@ | |||||||||||||||||||||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
using System.Diagnostics; | ||||||||||||||||||||||||
using System.Linq; | ||||||||||||||||||||||||
using System.Runtime.CompilerServices; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
namespace System.Collections.Generic | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
|
@@ -80,9 +80,112 @@ internal static T[] ToArray<T>(IEnumerable<T> source) | |||||||||||||||||||||||
return result; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
LargeArrayBuilder<T> builder = new(); | ||||||||||||||||||||||||
builder.AddRange(source); | ||||||||||||||||||||||||
return builder.ToArray(); | ||||||||||||||||||||||||
ToArrayHelper<T> helper = new ToArrayHelper<T>(initialCapacity: 4); | ||||||||||||||||||||||||
using (IEnumerator<T> e = source.GetEnumerator()) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
while (true) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
Span<T> span = helper.CurrentSpan; | ||||||||||||||||||||||||
for (int i = 0; i < span.Length; i++) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
if (e.MoveNext()) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
span[i] = e.Current; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
else | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
return helper.ToArray(i); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
helper.AllocateNextBlock(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// when smallest block size is 4, reaching Array.MaxLength size is "29". | ||||||||||||||||||||||||
// length, blockSize, totalSize | ||||||||||||||||||||||||
// 1 4 4 | ||||||||||||||||||||||||
// 2 8 12 | ||||||||||||||||||||||||
// 3 16 28 | ||||||||||||||||||||||||
// 4 32 60 | ||||||||||||||||||||||||
// 5 64 124 | ||||||||||||||||||||||||
// 6 128 252 | ||||||||||||||||||||||||
// 7 256 508 | ||||||||||||||||||||||||
// 8 512 1020 | ||||||||||||||||||||||||
// 9 1024 2044 | ||||||||||||||||||||||||
// 10 2048 4092 | ||||||||||||||||||||||||
// 11 4096 8188 | ||||||||||||||||||||||||
// 12 8192 16380 | ||||||||||||||||||||||||
// 13 16384 32764 | ||||||||||||||||||||||||
// 14 32768 65532 | ||||||||||||||||||||||||
// 15 65536 131068 | ||||||||||||||||||||||||
// 16 131072 262140 | ||||||||||||||||||||||||
// 17 262144 524284 | ||||||||||||||||||||||||
// 18 524288 1048572 | ||||||||||||||||||||||||
// 19 1048576 2097148 | ||||||||||||||||||||||||
// 20 2097152 4194300 | ||||||||||||||||||||||||
// 21 4194304 8388604 | ||||||||||||||||||||||||
// 22 8388608 16777212 | ||||||||||||||||||||||||
// 23 16777216 33554428 | ||||||||||||||||||||||||
// 24 33554432 67108860 | ||||||||||||||||||||||||
// 25 67108864 134217724 | ||||||||||||||||||||||||
// 26 134217728 268435452 | ||||||||||||||||||||||||
// 27 268435456 536870908 | ||||||||||||||||||||||||
// 28 536870912 1073741820 | ||||||||||||||||||||||||
// 29 1073741771 2147483591 <- reach Array.MaxLength(2147483591) | ||||||||||||||||||||||||
[InlineArray(29)] | ||||||||||||||||||||||||
private struct ArrayBlock<T> | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
#pragma warning disable CA1823 // Avoid unused private fields | ||||||||||||||||||||||||
#pragma warning disable IDE0044 // Add readonly modifier | ||||||||||||||||||||||||
#pragma warning disable IDE0051 // Remove unused private members | ||||||||||||||||||||||||
private T[] _array; | ||||||||||||||||||||||||
#pragma warning restore IDE0051 // Remove unused private members | ||||||||||||||||||||||||
#pragma warning restore IDE0044 // Add readonly modifier | ||||||||||||||||||||||||
#pragma warning restore CA1823 // Avoid unused private fields | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
private struct ToArrayHelper<T> | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
private int _index; | ||||||||||||||||||||||||
private int _count; | ||||||||||||||||||||||||
private T[] _currentBlock; | ||||||||||||||||||||||||
private ArrayBlock<T> _blocks; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
public ToArrayHelper(int initialCapacity) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
_blocks = default(ArrayBlock<T>); | ||||||||||||||||||||||||
_currentBlock = _blocks[0] = new T[initialCapacity]; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
public Span<T> CurrentSpan => _currentBlock; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
public void AllocateNextBlock() | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
_index++; | ||||||||||||||||||||||||
_count += _currentBlock.Length; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
int nextSize = unchecked(_currentBlock.Length * 2); | ||||||||||||||||||||||||
if (nextSize < 0 || Array.MaxLength < (_count + nextSize)) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
nextSize = Array.MaxLength - _count; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
_currentBlock = _blocks[_index] = new T[nextSize]; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
public T[] ToArray(int lastBlockCount) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
T[] array = GC.AllocateUninitializedArray<T>(_count + lastBlockCount); | ||||||||||||||||||||||||
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.
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 code for runtime/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs Lines 756 to 762 in 616f758
Given that AllocateUninitializedArray is marked as AggressiveInlining , there's likely no discernable perf impact here (modulo tuning on the threshold).
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. when length(_count + lastBlockCount) == 0, return |
||||||||||||||||||||||||
Span<T> dest = array.AsSpan(); | ||||||||||||||||||||||||
for (int i = 0; i < _index; i++) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
_blocks[i].CopyTo(dest); | ||||||||||||||||||||||||
dest = dest.Slice(_blocks[i].Length); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
Comment on lines
+181
to
+185
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.
Suggested change
This should remove the bounds checks here too. Not sure whether this is the best syntax possible here. 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. use |
||||||||||||||||||||||||
_currentBlock.AsSpan(0, lastBlockCount).CopyTo(dest); | ||||||||||||||||||||||||
return array; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} |
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.
Thanks. What would it take to augment your PR to the point where we could delete LargeArrayBuilder? That'd make me much more interested in this.
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.
LargeArrayBuilder is used in many places, and given that it includes numerous methods specific to certain scenarios, the amount of rewrites, including from the user's side, would likely be extensive. Ideally, we'd like to replace it entirely, but we want to exclude that from the initial implementation.
Using the ArrayPool as suggested in the option might not be advisable if we replace the LargeArrayBuilder.
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.
My concern is that these code paths are used in a bunch of places on common paths, and adding more generic types will increase code size. It'd be nice to get rid of the LargeArrayBuilder so that it's a net win for both throughput and size, rather than improving throughput at the expense of size.
There's no rush here as it's not going to make .NET 8, anyway, so there's plenty of time to make and evaluate the larger change.
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.
Thank you.
If this implementation is acceptable as a basic policy,
I'll take on the challenge of replacing the LargeArrayBuilder.
(Indeed, I was hoping it might make it into .NET 8! That would be great!)
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.
We'd support it in principle. Would you like to build on top of this PR or would you prefer to open a new one?
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.
I am willing to work on this PR.
NET 8 release seems to be coming soon, so I will do it from the branch there when it is released. ......
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.
main is the correct branch. Any such change will be for .NET 9.