Optimize storage for segment information #51383
Conversation
sharwell
force-pushed
the
faster-segments
branch
from
1b8feef
to
da87848
Compare
|
I was able to further optimize the net5.0 case by avoiding array variance checks. BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET Core SDK=5.0.103
[Host] : .NET Core 5.0.3 (CoreCLR 5.0.321.7212, CoreFX 5.0.321.7212), X64 RyuJIT
DefaultJob : .NET Core 5.0.3 (CoreCLR 5.0.321.7212, CoreFX 5.0.321.7212), X64 RyuJIT
The conditional code in the indexer does appear to have influenced the performance of |
sharwell
force-pushed
the
faster-segments
branch
from
387bc74
to
15e1fd3
Compare
| private static int SegmentSize | ||
| { | ||
| [MethodImpl(SegmentedArrayHelper.FastPathMethodImplOptions)] | ||
| get |
There was a problem hiding this comment.
get [](start = 12, length = 3)
Use expression bodies?
There was a problem hiding this comment.
I tried using [method: MethodImpl(...)] with expression bodied properties, but it didn't compile.
| using BenchmarkDotNet.Attributes; | ||
| using Microsoft.CodeAnalysis.Collections; | ||
|
|
||
| namespace IdeCoreBenchmarks |
There was a problem hiding this comment.
Really appreciate us checking in benchmarks with the improvements as you've done here.
| [MethodImpl(FastPathMethodImplOptions)] | ||
| internal static int GetSegmentSize<T>() | ||
| { | ||
| if (Unsafe.SizeOf<T>() == Unsafe.SizeOf<object>()) |
There was a problem hiding this comment.
This approach means our performance will differ between architectures. For instance the int will be considered a reference type 32 bit but not in 64 bit. Why aren't we using typeof(T).IsValueType here?
There was a problem hiding this comment.
The only real input to the whole segmenting algorithm is the value of Unsafe.SizeOf<T>(). For value types, generic specialization in the JIT means the value for any given value type will be inlined. For reference types, the JIT produces one common assembly implementation, so to avoid overhead we want all reference types to be using the same field reference for this value (both to avoid a lookup table for all the instantiations, and to improve the ability of the JIT to inline the indexer. Value types that happen to have the same size as a reference type are correct on both possible paths, so it doesn't matter which one we use.
typeof(T).IsValueType isn't a JIT intrinsic prior to net5.0, so if we try to use it for earlier targets we get something like a 20x regression. Unsafe.SizeOf<T>() behaves as an intrinsic (runtime constant) for all targets, so it gives good all-around results.
There were two situations where the previous code would allow net5.0 execution to access memory outside array bounds: 1. Replacement of an element in _items with a shorter array. Even though _items is indirectly exposed through SyncRoot, this is not a serious concern because replacement of an array element means unsafe/bad bytecode is already running in the process to perform the replacement. 2. A torn read of SegmentedArray<T> could allow a read of the _items field of one structure and the _length field of a different structure. This is a concern because it allows concurrent execution to reach a state where memory safety is violated without a precondition that unsafe code be running. This reverts commit 15e1fd3.
ghost
added this to the 
SegmentedArray<int>(and other unmanaged types) from 2.69x the cost ofint[]to 1.29x on .NET FrameworkSegmentedArray<object>(and other reference types), particularly on .NET FrameworkBefore this change
net472
netcoreapp3.1
net5.0
After this change
net472
netcoreapp3.1
net5.0