Make Array.Reverse faster #13962
Make Array.Reverse faster #13962
Conversation
|
@ahsonkhan This implements the improvement we have discussed over email. |
Array.Reverse showed up as hot method in profiles of real workloads (e.g. it is called from Json.NET frequently via List.Reverse)
jkotas
force-pushed
the
faster-reverse
branch
from
9356874 to
49bf9fe
Compare
| ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidOffLen); | ||
| Contract.EndContractBlock(); | ||
|
|
||
| ref T p = ref Unsafe.As<byte, T>(ref array.GetRawSzArrayData()); |
There was a problem hiding this comment.
Is there a way to do something similar to replace TrySZReverse in the non-generic Reverse?
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Array.cs#L1656
There was a problem hiding this comment.
Replacing TrySZReverse with C# implementation would be more profound change. I will leave it for a different day..
| array[j] = temp; | ||
| T temp = Unsafe.Add(ref p, i); | ||
| Unsafe.Add(ref p, i) = Unsafe.Add(ref p, j); | ||
| Unsafe.Add(ref p, j) = temp; |
There was a problem hiding this comment.
@jkotas, for my edification, what's the explanation for why this is faster? It's avoiding bounds checks that the JIT is currently unable to elide?
There was a problem hiding this comment.
It is avoiding bounds checks and covariance checks (for arrays of object references). The covariance checks are the more expensive ones of the two.
In theory, the JIT can notice that the source of the value is the same array as what it is assigned to and use that information to ship the covariance check ...
There was a problem hiding this comment.
List<T> is non-covariant so this could be done for Add etc?
There was a problem hiding this comment.
For List, you need to be careful about corrupted state that can lead to buffer overruns or type safety violations. Some of these bugs may be in the user code, but our general standard is that user code race should not turn into type or memory safety violations. I think a better way to deal with the covariance check overhead in general case like List is something like the covariance array wrapper (https://github.com/dotnet/corefx/issues/23689) - the unsafe code is just one line in the covariance array wrapper. The rest is safe.
There was a problem hiding this comment.
In theory, the JIT can notice that the source of the value is the same array as what it is assigned to and use that information to ship the covariance check ...
Is skipping the covariance check when source and destination are same, tracked somewhere?
There was a problem hiding this comment.
There was a problem hiding this comment.
If you load and store to the same array directly the jit already does this optimization (see for instance #14580, which enhanced an existing optimization). But if load and store are disconnected the jit may miss out.
@erozenfeld maybe this is something we could do more generally, once we have built up dataflow?
There was a problem hiding this comment.
mikedn commented on the current status of bounds check optimizations here: https://github.com/dotnet/coreclr/issues/15723
| i++; | ||
| j--; | ||
| } | ||
| Array.Reverse<object>(objArray, index, length); |
|
@dotnet-bot test Windows_NT x64 corefx_baseline |
7 participants
Array.Reverse showed up as hot method in profiles of real workloads (e.g. it is called from Json.NET frequently via List.Reverse)