-
Notifications
You must be signed in to change notification settings - Fork 6k
Guidelines for working with Memory<T> and Span<T> #8055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good @rpetrusha It's a clear explanation of these types and when to choose each.
I had a few suggestions for topics to introduce earlier that would make it more clear.
Overall, a great addition.
|
||
.NET Core includes a number of types that represent an arbitrary contiguous region of memory. .NET Core 2.0 introduced <xref:System.Span%601> and <xref:System.ReadOnlySpan%601>, which are lightweight memory buffers stored on the stack that can be backed by managed or unmanaged memory. Their stack-only storage makes these types unsuitable for a number of scenarios, including asynchronous method calls. .NET Core 2.1 adds a number of additional types, including <xref:System.Memory%601>, <xref:System.ReadOnlyMemory%601>, <xref:System.Buffers.IMemoryOwner%601>, and <xref:System.Buffers.MemoryPool%601>. Like <xref:System.Span%601>, <xref:Memory%601> and its related types can be backed by both managed and unmanaged memory. Unlike <xref:System.Span%601>, <xref:System.Memory%601> can be stored on the managed heap. | ||
|
||
Both <xref:System.Span%601> and <xref:System.Memory%601> are buffers of structured data that can be used in pipelines. That is, they are designed so that some or all of the data can be efficiently passed to components in the pipeline, which can process them and optionally modify the buffer. Because <xref:System.Memory%601> and its related types can be accessed by multiple components or by multiple threads, it is important that developers follow some standard usage guidelines to produce robust code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: double space "accessed by"
nit: "it is" => "it's"
|
||
Since buffers can be passed around between APIs, and since buffers can sometimes be accessed from multiple threads, it is important to consider lifetime management. There are three core concepts: | ||
|
||
- **Ownership**. The owner of a buffer instance is responsible for lifetime management, including destroying the buffer when it is no longer in use. All buffers have a single owner. Generally the owner is the component that created the buffer or that received the buffer from a factory. Ownership can also be transferred; **Component-** can relinquish control of the buffer to **Component-B**, at which point **Component-A** may no longer use the buffer, and **Component-B** becomes responsible for destroying the buffer when it is no longer in use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is => it's (2 occurrences)
"Component-" should be "Component-A"
|
||
- **Ownership**. The owner of a buffer instance is responsible for lifetime management, including destroying the buffer when it is no longer in use. All buffers have a single owner. Generally the owner is the component that created the buffer or that received the buffer from a factory. Ownership can also be transferred; **Component-** can relinquish control of the buffer to **Component-B**, at which point **Component-A** may no longer use the buffer, and **Component-B** becomes responsible for destroying the buffer when it is no longer in use. | ||
|
||
- **Consumption**. The consumer of a buffer instance is allowed to use the buffer instance by reading from it and possibly writing to it. Buffers can have one consumer at a time unless some external synchronization mechanism is provided. Note that the active consumer of a buffer is not necessarily the buffer's owner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not => isn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contractions don't seem to be my strong point, @BillWagner.
} | ||
``` | ||
|
||
The `Main` method creates the buffer (in this case an <xref:System.Span%601> instance) and so is its owner. Therefore, `Main` is responsible for destroying the buffer when it's no longer in use. It does this by calling the buffer's <xref:System.Span%601.Clear?displayProperty=nameWithType> method. (The <xref:System.Span%601.Clear> method here actually clears the buffer's memory; the <xref:System.Span%601> structure does not actually have a method that destroys the buffer.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not => doesn't
|
||
The buffer has two consumers, `WriteInt32ToBuffer` and `DisplayBufferToConsole`. There is only one consumer at a time (first `WriteInt32ToBuffer`, then `DisplayBufferToConsole`), and neither of the consumers owns the buffer. Note also that "consumer" in this context does not imply a read-only view of the buffer; consumers can modify the buffer's contents, as `WriteInt32ToBuffer does, if given a read/write view of the buffer. | ||
|
||
The `WriteInt32ToBuffer` method has a lease on (can consume) the buffer between the start of the method call and the time the method returns. Similarly, `DisplayBufferToConsole` has a lease on the buffer while it is executing, and the lease is released when the method unwinds. (There is no API for lease management; a "lease" is simply a conceptual matter.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider removing "simply" in the last clause.
void DisplayBufferToConsole(ReadOnlySpan<char> buffer); | ||
``` | ||
|
||
The `DisplayBufferToConsole` method now works with virtually every buffer type imaginable: `T[]`, [stackalloc](~/docs/csharp/language-reference/keywords/stackalloc.md), and so on. You can even pass a <xref:System.String> directly into it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of stackalloc feels wrong as written. Stackalloc needs some expression as its target, like an array or a span. Suggestion: "storage allocated with stackalloc"
|
||
The `WriteInt32ToBuffer` method has a lease on (can consume) the buffer between the start of the method call and the time the method returns. Similarly, `DisplayBufferToConsole` has a lease on the buffer while it is executing, and the lease is released when the method unwinds. (There is no API for lease management; a "lease" is simply a conceptual matter.) | ||
|
||
## Memory\<T> and the owner/consumer model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section took me a couple reads for the light bulbs to come on.
Consider starting this section by introducing 2 models: A model that enables ownership transfer, and a model that assumes a single owner. Then, introduce the IMemoryOwner interface as the mechanism to implement those models. (Its absence assumes the second model).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @BillWagner; this is a really helpful comment. I've addressed it and committed. I'll address your following comment either later this evening or tomorrow.
|
||
The method that initially creates the <xref:System.Memory%601> instance is the implicit owner of the buffer. Ownership cannot be transferred to any other component because there is no <xref:System.Buffers.IMemoryOwner%601> instance to facilitate the transfer. (As an alternative, you can also imagine that the runtime's garbage collector owns the buffer, and all methods just consume the buffer.) | ||
|
||
## Usage guidelines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a short section on Memory vs. Span and the lifetime / storage location implications. Many of the rules below make it clear that these distinctions are important. Introducing them and explaining them here would make the guidance more clear.
|
||
The `Main` method creates the buffer (in this case an <xref:System.Span%601> instance) and so is its owner. Therefore, `Main` is responsible for destroying the buffer when it's no longer in use. It does this by calling the buffer's <xref:System.Span%601.Clear?displayProperty=nameWithType> method. (The <xref:System.Span%601.Clear> method here actually clears the buffer's memory; the <xref:System.Span%601> structure doesn't actually have a method that destroys the buffer.) | ||
|
||
The buffer has two consumers, `WriteInt32ToBuffer` and `DisplayBufferToConsole`. There is only one consumer at a time (first `WriteInt32ToBuffer`, then `DisplayBufferToConsole`), and neither of the consumers owns the buffer. Note also that "consumer" in this context doesn't imply a read-only view of the buffer; consumers can modify the buffer's contents, as `WriteInt32ToBuffer does, if given a read/write view of the buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing closing backtick after WriteInt32ToBuffer
--- | ||
# Memory- and span-related types | ||
|
||
Starting with .NET Core 2.1, .NET includes a number of interrelated types that represent a contiguous, strongly-typed region of arbitrary memory. These include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Span
and Memory
are available on older frameworks too, through the System.Memory NuGet package, even if they are less efficient and have less supporting APIs. Is that worth a short mention here?
|
||
Starting with .NET Core 2.1, .NET includes a number of interrelated types that represent a contiguous, strongly-typed region of arbitrary memory. These include: | ||
|
||
- <xref:System.Span%601?displayProperty=nameWithType>, a contiguous region of memory that is allocated on the stack rather than the managed heap. This allows for enhanced performance. A <xref:System.Span%601> instance can be backed by an array of type \<T>, a <xref:System.String>, a buffer allocated with [stackalloc](~/docs/csharp/language-reference/keywords/stackalloc.md), or a pointer to unmanaged memory. Because it is allocated on the stack, it has a number of restrictions. For example, a field in a class cannot be of type <xref:System.Span%601>, nor can span be used in asynchronous operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.Span, a contiguous region of memory that is allocated on the stack rather than the managed heap.
Even though this is expanded on in the rest of the paragraph, I think it's very confusing to say this, because the memory itself does not have to be allocated on the stack. Maybe say something like "a type that can be used to access a contiguous region of memory"?
This allows for enhanced performance.
As I understand it, the stack-only restriction is not about performance (a regular struct would be just as efficient). It's about safety.
an array of type <T>
Why use angle brackets here (and also below)? Shouldn't it be "an array of type T
" instead?
Because it is allocated on the stack
Wouldn't it be better to say that it has to be allocated on the stack?
|
||
- <xref:System.Span%601?displayProperty=nameWithType>, a contiguous region of memory that is allocated on the stack rather than the managed heap. This allows for enhanced performance. A <xref:System.Span%601> instance can be backed by an array of type \<T>, a <xref:System.String>, a buffer allocated with [stackalloc](~/docs/csharp/language-reference/keywords/stackalloc.md), or a pointer to unmanaged memory. Because it is allocated on the stack, it has a number of restrictions. For example, a field in a class cannot be of type <xref:System.Span%601>, nor can span be used in asynchronous operations. | ||
|
||
- <xref:System.ReadOnlySpan%601?displayProperty=nameWithtype>, an immutable version of the <xref:System.Span%601> structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're using the same terms as .Net collections, then ReadOnlySpan
is not immutable, just read-only, because some other code can change the backing memory.
The same applies to ReadOnlyMemory
below.
|
||
- <xref:System.ReadOnlySpan%601?displayProperty=nameWithtype>, an immutable version of the <xref:System.Span%601> structure. | ||
|
||
- <xref:System.Memory%601?displayProperty=nameWithType>, a contiguous region of memory that is allocated on the managed heap rather than the stack. A <xref:System.Memory%601> instance can be backed by an array or type \<T> and a <xref:System.String>. Because it can be stored on the managed heap, <xref:System.Memory%601> has none of the limiations of <xref:System.Span%601>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a contiguous region of memory that is allocated on the managed heap rather than the stack
I think this is not correct: Memory
itself is a struct
, so it can be allocated on the stack. And the backing memory can't be stackalloc
ed, that's true, but it also doesn't have to be allocated on the managed heap, if you use a custom MemoryManager<T>
.
A Memory instance can be backed by an array or type <T> and a String.
Should this be: "A Memory instance can be backed by an array of type <T> or a String."?
It also does not mention MemoryManager<T>
, is the mention in its own item below sufficient?
|
||
- <xref:System.Buffers.MemoryManager%601>, an abstract base class that can be used to replace the implementation of <xref:System.Memory%601> so that <xref:System.Memory%601> can be backed by additional types, such as safe handles. <xref:System.Buffers.MemoryManager%601> is intended for advanced scenarios. | ||
|
||
- <xref:System.ArraySegment%601>, a wrapper for a particular number of array elements starting at a particular index. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever a reason to use ArraySegment
if you can use Memory
or Span
? If not, should that be mentioned here?
|
||
This rule also applies to code that calls factory methods like <xref:System.Buffers.MemoryPool%601.Rent%2A?displayProperty=nameWithType>. The caller becomes the owner of the returned <xref:System.Buffers.IMemoryOwner%601> and is responsible for disposing of the instance when finished. | ||
|
||
**Rule #8: If you have an IMemoryOwner\<T> parameter in your API surface are, you are accepting ownership of that instance.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: extra "are"
Any component that transfers ownership of the <xref:System.Buffers.IMemoryOwner%601> instance to a different component should no longer use that instance after the method call completes. | ||
|
||
> [!IMPORTANT] | ||
> If your constructor accepts <xref:System.Buffers.IMemoryOwner%601> as a parameter, it should implement <xref:System.IDisposable>, and your <xref:System.IDisposable.Dispose%2A> method should call <xref:System.Buffers.MemoryPool%601.Dispose%2A?displayProperty=nameWithType>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A constructor can't implement IDisposable
, so I think this should be something like:
If your constructor accepts IMemoryOwner as a parameter, the type should implement IDisposable
|
||
public unsafe int ManagedWrapper(Span<byte> data) | ||
{ | ||
fixed (byte* pbData = &MemoryMarshal.GetReference(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C# 7.3, you can write just fixed (byte* pbData = data)
.
I think it would make sense to include a note about that in this rule. (Along with a link on how to enable point releases of C#.)
|
||
**Rule #10: If you're wrapping an asynchronous p/invoke method, your API should accept Memory\<T> as a parameter.** | ||
|
||
Since you cannot use the [`fixed`](~/docs/csharp/language-reference/keywords/fixed-statement.md) keyword across asynchronous operations, you use the <xref:System.Memory%601.Pin%2A?displayProperty=nameWithType> method to pin <xref:System.Memory%601> instances, regardless of the kind of contiguous memory the instance represents. The following example shows how to do use this API to perform an asynchronous p/invoke call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: do use
var state = new MyCompletedCallbackState { | ||
Tcs = tcs | ||
}; | ||
var pState = (IntPtr)GCHandle.Alloc(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be: var pState = (IntPtr)GCHandle.Alloc(state);
.
I approved the changes, as I had no further issues @rpetrusha I'll let you work through with @mairaw and others if more needs to be done. |
Guidelines for working with Memory<T> and Span<T>
Fixes #4823
@ahsonkhan @stephentoub @GrabYourPitchforks