Combining the T[] and OwnedBuffer<T> fields of Buffer<T> into a single object #1634
Conversation
|
No performance implications due to using |
There is insignificant difference in performance (if it gets inlined). The result below is an average of 3 runs:
There are performance implications if it doesn't get inlined. In the case of accessing Span property, it gets twice as slow. |
| @@ -14,16 +14,14 @@ namespace System | |||
| [DebuggerTypeProxy(typeof(BufferDebuggerView<>))] | |||
| public struct Buffer<T> | |||
| { | |||
| readonly OwnedBuffer<T> _owner; | |||
| readonly T[] _array; | |||
| readonly object _arrayOrOwnedBuffer; | |||
There was a problem hiding this comment.
It would be good to add a comment that explains how this works, i.e. that _index high order bit is used to identify the modes.
|
Looks good to me. |
|
@dotnet-bot test Innerloop Windows_NT Debug Build and Test |
|
@dotnet-bot test this please |
|
cc @VSadov, we are still seeing the build failures. |
|
@dotnet-bot test Innerloop OSX10.12 Debug Build and Test |
|
perhaps revering to #1637 might help. At this point I think I can hit a repro on my machine. Not sure if that is the same bug, but it looks like it is, so no need to continue breaking your PRs. |
|
@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test |
| @@ -69,24 +70,19 @@ public Buffer(T[] array, int start, int length) | |||
| if ((uint)start > (uint)array.Length || (uint)length > (uint)(array.Length - start)) | |||
There was a problem hiding this comment.
nit:
It looks like:
if ((uint)length > array.Length - (uint)start) is the same, because if (start > array.Length) -> array.Length - (uint)start less than zero and so less than any uint.
Also, Why casting to uint is necessary?
There was a problem hiding this comment.
It looks like:
if ((uint)length > array.Length - (uint)start)is the same, because if (start > array.Length) -> array.Length - (uint)start less than zero and so less than any uint.
I don't understand what you mean. It is the same as what? The two conditions are or'd together.
So, if ((uint)start > (uint)array.Length) is true, the second condition gets short-circuited. Only if the first condition is false, i.e. start is <= array.Length, would we run the second condition.
Essentially, we need the following behaviour:
var buffer = new Buffer(new byte[100], 50, 50); // works
var buffer = new Buffer(new byte[100], 101, 1); // it should throw
var buffer = new Buffer(new byte[100], 50, 60); // it should throw, this won't throw if I remove the second conditionThe checks in the Buffer<T> constructor implementation are inspired by Span<T>:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Span.cs#L85
Also, Why casting to uint is necessary?
I believe that is an optimization to try and help the JIT eliminate the redundant bound check. @jkotas, can you confirm?
Although, I don't see any reduction in number of instructions in the disassembly.
See #1616 (comment)
There was a problem hiding this comment.
Why casting to uint is necessary?
(uint)start > (uint)array.Length tests both a negative start and start being larger than Length as a negative int cast to a uint is larger than int.MaxValue.
Now it is known start is within bounds, (uint)length > (uint)(array.Length - start)) tests whether length is negative or larger than available.
So 4 comparisons are done in the statement, using 2 comparisons.
Not using the cast to uint the equivalent would be:
if (
start < 0 || start > array.Length ||
length < 0 || length > (array.Length - start)
)
|
@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test |
cc @KrzysztofCwalina, @shiftylogic
Using the first bit of index (which is unused since index >= 0) to discern whether the
readonly object _arrayOrOwnedBufferfield is an array or an owned buffer.