Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions src/mscorlib/shared/System.Private.CoreLib.Shared.projitems
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,10 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\ArrayPool.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\ArrayPoolEventSource.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\ConfigurableArrayPool.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\IRetainable.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\IMemoryOwner.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\IPinnable.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\MemoryHandle.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\OwnedMemory.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\MemoryManager.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\TlsOverPerCoreLockedStacksArrayPool.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Buffers\Utilities.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Byte.cs" />
Expand Down
17 changes: 17 additions & 0 deletions src/mscorlib/shared/System/Buffers/IMemoryOwner.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace System.Buffers
{
/// <summary>
/// Owner of Memory<typeparamref name="T"/> that is responsible for disposing the underlying memory appropriately.
/// </summary>
public interface IMemoryOwner<T> : IDisposable
{
/// <summary>
/// Returns a Memory<typeparamref name="T"/>.
/// </summary>
Memory<T> Memory { get; }
}
}
25 changes: 25 additions & 0 deletions src/mscorlib/shared/System/Buffers/IPinnable.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace System.Buffers
{
/// <summary>
/// Provides a mechanism for pinning and unpinning objects to prevent the GC from moving them.
/// </summary>
public interface IPinnable
{
/// <summary>
/// Call this method to indicate that the IPinnable object can not be moved by the garbage collector.
/// The address of the pinned object can be taken.
/// <param name="elementIndex">The offset to the element within the memory at which the returned <see cref="MemoryHandle"/> points to.</param>
/// </summary>
MemoryHandle Pin(int elementIndex);

/// <summary>
/// Call this method to indicate that the IPinnable object no longer needs to be pinned.
/// The garbage collector is free to move the object now.
/// </summary>
void Unpin();
}
}
26 changes: 0 additions & 26 deletions src/mscorlib/shared/System/Buffers/IRetainable.cs

This file was deleted.

29 changes: 12 additions & 17 deletions src/mscorlib/shared/System/Buffers/MemoryHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,53 +12,48 @@ namespace System.Buffers
/// </summary>
public unsafe struct MemoryHandle : IDisposable
{
private IRetainable _retainable;
private void* _pointer;
private GCHandle _handle;
private IPinnable _pinnable;

/// <summary>
/// Creates a new memory handle for the memory.
/// </summary>
/// <param name="retainable">reference to manually managed object</param>
/// <param name="pointer">pointer to memory, or null if a pointer was not provided when the handle was created</param>
/// <param name="pointer">pointer to memory</param>
/// <param name="pinnable">reference to manually managed object, or default if there is no memory manager</param>
/// <param name="handle">handle used to pin array buffers</param>
[CLSCompliant(false)]
public MemoryHandle(IRetainable retainable, void* pointer = null, GCHandle handle = default(GCHandle))
public MemoryHandle(void* pointer, GCHandle handle = default, IPinnable pinnable = default)
{
_retainable = retainable;
_pointer = pointer;
_handle = handle;
_pinnable = pinnable;
}

/// <summary>
/// Returns the pointer to memory, or null if a pointer was not provided when the handle was created.
/// Returns the pointer to memory, where the memory is assumed to be pinned and hence the address won't change.
/// </summary>
[CLSCompliant(false)]
public void* Pointer => _pointer;

/// <summary>
/// Returns false if the pointer to memory is null.
/// Frees the pinned handle and releases IPinnable.
/// </summary>
public bool HasPointer => _pointer != null;

/// <summary>
/// Frees the pinned handle and releases IRetainable.
/// </summary>
public void Dispose()
public void Dispose()
{
if (_handle.IsAllocated)
{
_handle.Free();
}

if (_retainable != null)
if (_pinnable != null)
{
_retainable.Release();
_retainable = null;
_pinnable.Unpin();
_pinnable = null;
}

_pointer = null;
}

}
}
}
66 changes: 66 additions & 0 deletions src/mscorlib/shared/System/Buffers/MemoryManager.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Runtime;
using System.Runtime.CompilerServices;

namespace System.Buffers
{
/// <summary>
/// Manager of Memory<typeparamref name="T"/> that provides the implementation.
/// </summary>
public abstract class MemoryManager<T> : IMemoryOwner<T>, IPinnable
{
/// <summary>
/// The number of items in the Memory<typeparamref name="T"/>.
/// </summary>
public virtual int Length => GetSpan().Length;
Copy link
Member

Choose a reason for hiding this comment

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

With this change; any reason for this to be virtual?

Copy link
Member

Choose a reason for hiding this comment

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

Probably to avoid the GetSpan() call?

Copy link
Member

Choose a reason for hiding this comment

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

Could have done

public int Length { get; }

protected MemoryManager(int length)
{
    Length = length;
}

public Memory<T> Memory => new Memory<T>(this, 0, Length);

Unless its valid that derived classes of MemoryManager can change in length on the go?

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Mar 30, 2018

Choose a reason for hiding this comment

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

@benaadams There are scenarios where this can change; e.g., if the factory pools and reuses these instances. We can provide a default implementation of this logic via reading the value from GetSpan(), but as a perf optimization implementations may want to provide the value in a more direct manner.

Copy link

Choose a reason for hiding this comment

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

Why would it change in case of a pool? We usually don't return exact size requested.

Copy link
Member

Choose a reason for hiding this comment

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

@benaadams @GrabYourPitchforks just made me aware of the fact that this type will rarely be used directly because the MemoryPool returns IMemoryOwner<T>, so really none of this matters. Though as an implementer of MemoryManager<T>, I think it's confusing to expose length. So lets try to come up with something concrete.

Copy link
Member

Choose a reason for hiding this comment

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

K so Memory won't inline due to the interface.

However as the implementer you will have to get a Length somewhere and possibly an Offset, so can stick it on the type and have everything else inline and have a good performance implementation with no additional virtuals or the need to implement the two methods?

public abstract class MemoryManager<T> : IMemoryOwner<T>, IPinnable
{
    protected int Offset { get; set; }
    protected int Length { get; set; }

    protected MemoryManager()
    {
    }
    
    protected MemoryManager(int length)
    {
        Length = length;
    }

    protected MemoryManager(int offset, int length)
    {
        Offset = offset;
        Length = length;
    }

    public Memory<T> Memory => new Memory<T>(this, Offset, Length);
    
    // Rest as now

Copy link
Member

@benaadams benaadams Mar 30, 2018

Choose a reason for hiding this comment

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

Or kill Length and force the implementation

public abstract class MemoryManager<T> : IMemoryOwner<T>, IPinnable
{
    public abstract Memory<T> Memory { get; }
    
    // Rest as now

Passing off the validation of offset and length to derived class

public class MyMemory : MemoryManager<byte>
{
    private int Offset { get; set; }
    private int Length { get; set; }
    
    public MyMemory(...)
    {
        // ...
    }
    
    public override Memory<T> Memory => new Memory<T>(this, Offset, Length);
} 

Copy link
Member

Choose a reason for hiding this comment

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

@GrabYourPitchforks making it abstract?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. In the interest of time, I am going to merge the change as is and we can consider changing this separately.


/// <summary>
/// Returns a Memory<typeparamref name="T"/>.
/// </summary>
public virtual Memory<T> Memory => new Memory<T>(this, 0, Length);

/// <summary>
/// Returns a span wrapping the underlying memory.
/// </summary>
public abstract Span<T> GetSpan();

/// <summary>
/// Returns a handle to the memory that has been pinned and hence its address can be taken.
/// <param name="elementIndex">The offset to the element within the memory at which the returned <see cref="MemoryHandle"/> points to. (default = 0)</param>
/// </summary>
public abstract MemoryHandle Pin(int elementIndex = 0);

/// <summary>
/// Lets the garbage collector know that the object is free to be moved now.
/// </summary>
public abstract void Unpin();

/// <summary>
/// Returns an array segment.
/// <remarks>Returns the default array segment if not overriden.</remarks>
/// </summary>
protected internal virtual bool TryGetArray(out ArraySegment<T> segment)
{
segment = default;
return false;
}

/// <summary>
/// Implements IDisposable.
/// </summary>
void IDisposable.Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}

/// <summary>
/// Clean up of any leftover managed and unmanaged resources.
/// </summary>
protected abstract void Dispose(bool disposing);

}
}
95 changes: 0 additions & 95 deletions src/mscorlib/shared/System/Buffers/OwnedMemory.cs

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ private sealed class MemoryFileStreamCompletionSource : FileStreamCompletionSour
internal MemoryFileStreamCompletionSource(FileStream stream, int numBufferedBytes, ReadOnlyMemory<byte> memory) :
base(stream, numBufferedBytes, bytes: null) // this type handles the pinning, so null is passed for bytes
{
_handle = memory.Retain(pin: true);
_handle = memory.Pin();
}

internal override void ReleaseNativeResource()
Expand Down
Loading