Simplify non-generic ArrayEnumerator #51351
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| @@ -124,6 +124,77 @@ public static void Copy(Array sourceArray, long sourceIndex, Array destinationAr | |||
| Copy(sourceArray, isourceIndex, destinationArray, idestinationIndex, ilength); | |||
| } | |||
|
|
|||
| #if !MONO | |||
| // The various Get values... | |||
| public object? GetValue(params int[] indices) | |||
There was a problem hiding this comment.
@CoffeeFlux I think this can be shared with Mono, but I am not sure about the best way to implement the GetFlattenedIndex method for Mono.
There was a problem hiding this comment.
I gave it a shot. Let me know what you think
There was a problem hiding this comment.
@lambdageek please have a look as well
There was a problem hiding this comment.
@lambdageek Do you plan to take a look?
There was a problem hiding this comment.
@jkotas sorry about that, LGTM
| return new SZArrayEnumerator(this); | ||
| else | ||
| return new ArrayEnumerator(this, lowerBound, Length); | ||
| return new ArrayEnumerator(this); |
There was a problem hiding this comment.
This is the main point of the change - use the same implementation for both single and multi-dimensional arrays.
jkotas
requested review from
CoffeeFlux,
lambdageek,
thaystg and
vargaz
as code owners
|
Does it need any perf validation? |
|
The performance of these non-generic enumerators and accessors does not matter a whole lot. They are slow by construction. Anyway, this change makes them faster. For example, This change moves more of the implementation from C/C++ into C#, so the perf results may vary for modalities where the managed code executes slower than native code, such as wasm interpretter. I assume that we do not care. |
correct |
|
Tagging subscribers to this area: @tannergooding Issue Details
|
|
@GrabYourPitchforks Could you please review the CoreCLR part of the change? |
| if (RuntimeHelpers.GetMethodTable(this)->IsMultiDimensionalArray) | ||
| { | ||
| ref int bounds = ref RuntimeHelpers.GetMultiDimensionalArrayBounds(this); | ||
| nint flattenedIndex = 0; |
There was a problem hiding this comment.
Nit: I think nuint makes a bit more sense than nint since we're talking about absolute offsets, and absolute offsets can never be negative. But if the convention is to use nint, so be it.
There was a problem hiding this comment.
Yeah, I need signed int for the iterator. So this really becomes a question of where you choose to place the casts.
I prefer signed int in situations like this, both in C/C++ and C#. There has been a lot of written about signed vs. unsigned. For example, https://google.github.io/styleguide/cppguide.html#Integer_Types that is aligned with my thinking on this.
| @@ -139,11 +38,12 @@ public bool MoveNext() | |||
| { | |||
| get | |||
| { | |||
| if (_index < 0) | |||
| nint index = _index; | |||
| if (index < 0) | |||
There was a problem hiding this comment.
Does it make sense to reorder these with success first (predicted taken) and failures later?
if ((nuint)index < (nuint)_array.LongLength)) { return _array.InternalGetValue(index); }
else if (not_yet_started) { /* ... */ }
else { /* ... */ }There was a problem hiding this comment.
I have rearranged it so that the flow looks like the flow of the generic enumerator in this file.
I have not done the manual predicten taken part. That's a problem for the JIT to deal with.
ghost
locked as resolved and limited conversation to collaborators
No description provided.