Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

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

Merged
ahsonkhan merged 11 commits into from
Jun 28, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
89b3f45
Changing Buffer<T> fields to void* and object
ahsonkhan Jun 20, 2017
16182d4
Fixing Buffer Span and Retain to account for object being null
ahsonkhan Jun 22, 2017
97e7263
WIP - fixing impl of retain
ahsonkhan Jun 22, 2017
b70cf9a
Using last bit of index to differentiate between array & ownedbuffer
ahsonkhan Jun 27, 2017
690c089
Merge branch 'master' of https://github.com/dotnet/corefxlab into Buf…
ahsonkhan Jun 27, 2017
a29cfa8
Fixing use of index by using bitmask to ignore first bit
ahsonkhan Jun 27, 2017
26b8bc6
Removing unnecessary unsafe keyword since buffer no longer contains v…
ahsonkhan Jun 27, 2017
5261df6
Adding owned buffer test, use after gc collection
ahsonkhan Jun 27, 2017
b1af4fe
Aggressive inlining span property
ahsonkhan Jun 27, 2017
3a794e4
Addressing PR comment
ahsonkhan Jun 27, 2017
c379c93
Tweaking test and comment
ahsonkhan Jun 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 53 additions & 45 deletions src/System.Buffers.Primitives/System/Buffers/Buffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,19 @@ namespace System
[DebuggerTypeProxy(typeof(BufferDebuggerView<>))]
public struct Buffer<T>
{
readonly OwnedBuffer<T> _owner;
readonly T[] _array;
// The highest order bit of _index is used to discern whether _arrayOrOwnedBuffer is an array or an owned buffer
// if (_index >> 31) == 1, object _arrayOrOwnedBuffer is an OwnedBuffer<T>
// else, object _arrayOrOwnedBuffer is a T[]
readonly object _arrayOrOwnedBuffer;
readonly int _index;
readonly int _length;

private const int bitMask = 0x7FFFFFFF;

internal Buffer(OwnedBuffer<T> owner, int index, int length)
{
_array = null;
_owner = owner;
_index = index;
_arrayOrOwnedBuffer = owner;
_index = index | (1 << 31); // Before using _index, check if _index < 0, then 'and' it with bitMask
_length = length;
}

Expand All @@ -35,8 +38,7 @@ public Buffer(T[] array)
if (default(T) == null && array.GetType() != typeof(T[]))
BufferPrimitivesThrowHelper.ThrowArrayTypeMismatchException(typeof(T));

_array = array;
_owner = null;
_arrayOrOwnedBuffer = array;
_index = 0;
_length = array.Length;
}
Expand All @@ -53,8 +55,7 @@ public Buffer(T[] array, int start)
if ((uint)start > (uint)arrayLength)
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

_array = array;
_owner = null;
_arrayOrOwnedBuffer = array;
_index = start;
_length = arrayLength - start;
}
Expand All @@ -69,24 +70,19 @@ public Buffer(T[] array, int start, int length)
if ((uint)start > (uint)array.Length || (uint)length > (uint)(array.Length - start))
Copy link
Contributor

@Vedin Vedin Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ahsonkhan ahsonkhan Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@benaadams benaadams Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

_array = array;
_owner = null;
_arrayOrOwnedBuffer = array;
_index = start;
_length = length;
}

private Buffer(OwnedBuffer<T> owner, T[] array, int index, int length)
{
_array = array;
_owner = owner;
_index = index;
_length = length;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator ReadOnlyBuffer<T>(Buffer<T> buffer)
{
return new ReadOnlyBuffer<T>(buffer._owner, buffer._array, buffer._index, buffer._length);
// There is no need to 'and' _index by the bit mask here
// since the constructor will set the highest order bit again anyway
if (buffer._index < 0)
return new ReadOnlyBuffer<T>(Unsafe.As<OwnedBuffer<T>>(buffer._arrayOrOwnedBuffer), buffer._index, buffer._length);
return new ReadOnlyBuffer<T>(Unsafe.As<T[]>(buffer._arrayOrOwnedBuffer), buffer._index, buffer._length);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand All @@ -113,7 +109,11 @@ public Buffer<T> Slice(int start)
if ((uint)start > (uint)_length)
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

return new Buffer<T>(_owner, _array, _index + start, _length - start);
// There is no need to and _index by the bit mask here
// since the constructor will set the highest order bit again anyway
if (_index < 0)
return new Buffer<T>(Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer), _index + start, _length - start);
return new Buffer<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index + start, _length - start);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand All @@ -122,14 +122,21 @@ public Buffer<T> Slice(int start, int length)
if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

return new Buffer<T>(_owner, _array, _index + start, length);
// There is no need to 'and' _index by the bit mask here
// since the constructor will set the highest order bit again anyway
if (_index < 0)
return new Buffer<T>(Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer), _index + start, length);
return new Buffer<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index + start, length);
}

public Span<T> Span
{
get {
if (_array != null) return new Span<T>(_array, _index, _length);
return _owner.AsSpan(_index, _length);
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get
{
if (_index < 0)
return Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).AsSpan(_index & bitMask, _length);
return new Span<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index, _length);
}
}

Expand All @@ -138,13 +145,13 @@ public BufferHandle Retain(bool pin = false)
BufferHandle bufferHandle;
if (pin)
{
if (_owner != null)
if (_index < 0)
{
bufferHandle = _owner.Pin(_index);
bufferHandle = Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).Pin(_index & bitMask);
}
else
{
var handle = GCHandle.Alloc(_array, GCHandleType.Pinned);
var handle = GCHandle.Alloc(Unsafe.As<T[]>(_arrayOrOwnedBuffer), GCHandleType.Pinned);
unsafe
{
var pointer = OwnedBuffer<T>.Add((void*)handle.AddrOfPinnedObject(), _index);
Expand All @@ -154,26 +161,32 @@ public BufferHandle Retain(bool pin = false)
}
else
{
if (_owner != null)
if (_index < 0)
{
Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).Retain();
bufferHandle = new BufferHandle(Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer));
}
else
{
_owner.Retain();
bufferHandle = new BufferHandle(null);
}
bufferHandle = new BufferHandle(_owner);
}
return bufferHandle;
}

public bool TryGetArray(out ArraySegment<T> arraySegment)
{
if (_owner != null && _owner.TryGetArray(out var segment))
if (_index < 0)
{
arraySegment = new ArraySegment<T>(segment.Array, segment.Offset + _index, _length);
return true;
if (Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).TryGetArray(out var segment))
{
arraySegment = new ArraySegment<T>(segment.Array, segment.Offset + (_index & bitMask), _length);
return true;
}
}

if (_array != null)
else
{
arraySegment = new ArraySegment<T>(_array, _index, _length);
arraySegment = new ArraySegment<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index, _length);
return true;
}

Expand Down Expand Up @@ -213,9 +226,8 @@ public override bool Equals(object obj)
public bool Equals(Buffer<T> other)
{
return
_array == other._array &&
_owner == other._owner &&
_index == other._index &&
_arrayOrOwnedBuffer == other._arrayOrOwnedBuffer &&
(_index & bitMask) == (other._index & bitMask) &&
_length == other._length;
}

Expand All @@ -232,11 +244,7 @@ public bool Equals(Buffer<T> other)
[EditorBrowsable( EditorBrowsableState.Never)]
public override int GetHashCode()
{
if (_owner != null)
{
return HashingHelper.CombineHashCodes(_owner.GetHashCode(), _index.GetHashCode(), _length.GetHashCode());
}
return HashingHelper.CombineHashCodes(_array.GetHashCode(), _index.GetHashCode(), _length.GetHashCode());
return HashingHelper.CombineHashCodes(_arrayOrOwnedBuffer.GetHashCode(), (_index & bitMask).GetHashCode(), _length.GetHashCode());
}
}
}
96 changes: 50 additions & 46 deletions src/System.Buffers.Primitives/System/Buffers/ReadOnlyBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,19 @@ namespace System
[DebuggerTypeProxy(typeof(ReadOnlyBufferDebuggerView<>))]
public struct ReadOnlyBuffer<T>
{
readonly OwnedBuffer<T> _owner;
readonly T[] _array;
// The highest order bit of _index is used to discern whether _arrayOrOwnedBuffer is an array or an owned buffer
// if (_index >> 31) == 1, object _arrayOrOwnedBuffer is an OwnedBuffer<T>
// else, object _arrayOrOwnedBuffer is a T[]
readonly object _arrayOrOwnedBuffer;
readonly int _index;
readonly int _length;

internal ReadOnlyBuffer(OwnedBuffer<T> owner,int index, int length)
private const int bitMask = 0x7FFFFFFF;

internal ReadOnlyBuffer(OwnedBuffer<T> owner, int index, int length)
{
_array = null;
_owner = owner;
_index = index;
_arrayOrOwnedBuffer = owner;
_index = index | (1 << 31); // Before using _index, check if _index < 0, then 'and' it with bitMask
_length = length;
}

Expand All @@ -33,8 +36,7 @@ public ReadOnlyBuffer(T[] array)
if (array == null)
BufferPrimitivesThrowHelper.ThrowArgumentNullException(ExceptionArgument.array);

_array = array;
_owner = null;
_arrayOrOwnedBuffer = array;
_index = 0;
_length = array.Length;
}
Expand All @@ -49,8 +51,7 @@ public ReadOnlyBuffer(T[] array, int start)
if ((uint)start > (uint)arrayLength)
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

_array = array;
_owner = null;
_arrayOrOwnedBuffer = array;
_index = start;
_length = arrayLength - start;
}
Expand All @@ -63,20 +64,11 @@ public ReadOnlyBuffer(T[] array, int start, int length)
if ((uint)start > (uint)array.Length || (uint)length > (uint)(array.Length - start))
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

_array = array;
_owner = null;
_arrayOrOwnedBuffer = array;
_index = start;
_length = length;
}

internal ReadOnlyBuffer(OwnedBuffer<T> owner, T[] array, int index, int length)
{
_array = array;
_owner = owner;
_index = index;
_length = length;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator ReadOnlyBuffer<T>(T[] array)
{
Expand All @@ -101,7 +93,11 @@ public ReadOnlyBuffer<T> Slice(int start)
if ((uint)start > (uint)_length)
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

return new ReadOnlyBuffer<T>(_owner, _array, _index + start, _length - start);
// There is no need to 'and' _index by the bit mask here
// since the constructor will set the highest order bit again anyway
if (_index < 0)
return new ReadOnlyBuffer<T>(Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer), _index + start, _length - start);
return new ReadOnlyBuffer<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index + start, _length - start);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand All @@ -110,14 +106,21 @@ public ReadOnlyBuffer<T> Slice(int start, int length)
if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
BufferPrimitivesThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

return new ReadOnlyBuffer<T>(_owner, _array, _index + start, length);
// There is no need to 'and' _index by the bit mask here
// since the constructor will set the highest order bit again anyway
if (_index < 0)
return new ReadOnlyBuffer<T>(Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer), _index + start, length);
return new ReadOnlyBuffer<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index + start, length);
}

public ReadOnlySpan<T> Span
{
get {
if (_array != null) return new ReadOnlySpan<T>(_array, _index, _length);
return _owner.AsSpan(_index, _length);
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get
{
if (_index < 0)
return Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).AsSpan(_index & bitMask, _length);
return new ReadOnlySpan<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index, _length);
}
}

Expand All @@ -126,13 +129,13 @@ public BufferHandle Retain(bool pin = false)
BufferHandle bufferHandle;
if (pin)
{
if (_owner != null)
if (_index < 0)
{
bufferHandle = _owner.Pin(_index);
bufferHandle = Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).Pin(_index & bitMask);
}
else
{
var handle = GCHandle.Alloc(_array, GCHandleType.Pinned);
var handle = GCHandle.Alloc(Unsafe.As<T[]>(_arrayOrOwnedBuffer), GCHandleType.Pinned);
unsafe
{
var pointer = OwnedBuffer<T>.Add((void*)handle.AddrOfPinnedObject(), _index);
Expand All @@ -142,11 +145,15 @@ public BufferHandle Retain(bool pin = false)
}
else
{
if (_owner != null)
if (_index < 0)
{
Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).Retain();
bufferHandle = new BufferHandle(Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer));
}
else
{
_owner.Retain();
bufferHandle = new BufferHandle(null);
}
bufferHandle = new BufferHandle(_owner);
}
return bufferHandle;
}
Expand All @@ -156,19 +163,21 @@ public BufferHandle Retain(bool pin = false)
[EditorBrowsable(EditorBrowsableState.Never)]
public bool DangerousTryGetArray(out ArraySegment<T> arraySegment)
{
if (_owner != null && _owner.TryGetArray(out var segment))
if (_index < 0)
{
arraySegment = new ArraySegment<T>(segment.Array, segment.Offset + _index, _length);
return true;
if (Unsafe.As<OwnedBuffer<T>>(_arrayOrOwnedBuffer).TryGetArray(out var segment))
{
arraySegment = new ArraySegment<T>(segment.Array, segment.Offset + (_index & bitMask), _length);
return true;
}
}

if (_array != null)
else
{
arraySegment = new ArraySegment<T>(_array, _index, _length);
arraySegment = new ArraySegment<T>(Unsafe.As<T[]>(_arrayOrOwnedBuffer), _index, _length);
return true;
}

arraySegment = default(ArraySegment<T>);
arraySegment = default;
return false;
}

Expand Down Expand Up @@ -202,9 +211,8 @@ public override bool Equals(object obj)
public bool Equals(ReadOnlyBuffer<T> other)
{
return
_array == other._array &&
_owner == other._owner &&
_index == other._index &&
_arrayOrOwnedBuffer == other._arrayOrOwnedBuffer &&
(_index & bitMask) == (other._index & bitMask) &&
_length == other._length;
}

Expand All @@ -221,11 +229,7 @@ public bool Equals(ReadOnlyBuffer<T> other)
[EditorBrowsable(EditorBrowsableState.Never)]
public override int GetHashCode()
{
if (_owner != null)
{
return HashingHelper.CombineHashCodes(_owner.GetHashCode(), _index.GetHashCode(), _length.GetHashCode());
}
return HashingHelper.CombineHashCodes(_array.GetHashCode(), _index.GetHashCode(), _length.GetHashCode());
return HashingHelper.CombineHashCodes(_arrayOrOwnedBuffer.GetHashCode(), (_index & bitMask).GetHashCode(), _length.GetHashCode());
}
}
}
Loading