Combining the T[] and OwnedBuffer<T> fields of Buffer<T> into a single object

[ahsonkhan]

cc @KrzysztofCwalina, @shiftylogic

Using the first bit of index (which is unused since index >= 0) to discern whether the readonly object _arrayOrOwnedBuffer field is an array or an owned buffer.

[benaadams]

No performance implications due to using Unsafe.As?

[ahsonkhan]

No performance implications due to using Unsafe.As?

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.

[KrzysztofCwalina]

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.

[KrzysztofCwalina]

Looks good to me.

[ahsonkhan]

@dotnet-bot test Innerloop Windows_NT Debug Build and Test
@dotnet-bot test Innerloop Ubuntu16.04 Debug Build and Test
@dotnet-bot test Innerloop OSX10.12 Debug Build and Test

[shiftylogic]

@dotnet-bot test this please

[ahsonkhan]

cc @VSadov, we are still seeing the build failures.

[ahsonkhan]

@dotnet-bot test Innerloop OSX10.12 Debug Build and Test
@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test

[VSadov]

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.

[ahsonkhan]

@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test

[Vedin]

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?

[ahsonkhan]

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 condition

The 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)

[benaadams]

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)
   )

[ahsonkhan]

@dotnet-bot test Innerloop Ubuntu16.04 Release Build and Test