Implement and use MultiArrayBuffer #44980
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsMultiArrayBuffer is a struct that manages a sliding buffer, where bytes be added at the end and removed at the beginning, similar to existing ArrayBuffer. Unlike ArrayBuffer, it manages a list of fixed-size (16K) blocks internally. This is most useful for scenarios where the buffer may grow large, and thus managing it as a single array is probably not efficient. MultiArrayBuffer is now used in the HTTP2 response buffer code, as well as in StreamBuffer (used by ConnectedStreams and mock QUIC provider). Also, change stream conformance tests to not use Assert.Equals for buffer comparsion, as it is very, very slow. This reduces the time of outerloop StreamConformanceTests by approximately 99%. @stephentoub @dotnet/ncl
|
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| _blocks = null; | ||
| _blockCount = 0; | ||
| _activeStart = 0; | ||
| _availableStart = 0; |
There was a problem hiding this comment.
Calling the default-ctor or this = default produces better zeroing code.
Don't know if it's worth it.
There was a problem hiding this comment.
Maybe. I could add a factory method here that does the check, and change callers to use it. Let me look at it...
| { | ||
| get | ||
| { | ||
| if (index < 0 || index >= _length) |
There was a problem hiding this comment.
| if (index < 0 || index >= _length) | |
| if ((uint)index >= (uint)_length) |
Saves one cmp.
| Debug.Assert(byteCount >= 0); | ||
| Debug.Assert(byteCount <= ActiveMemory.Length, $"MultiArrayBuffer.Discard: Expected byteCount={byteCount} <= {ActiveMemory.Length}"); | ||
|
|
||
| int oldStartBlock = _activeStart / BlockSize; |
There was a problem hiding this comment.
JIT produces optimized division for this. Casting to uint, then the JIT emits bit-shifts.
| int oldStartBlock = _activeStart / BlockSize; | |
| int oldStartBlock = (int)((uint)_activeStart / BlockSize); |
But code looks uglier.
The same on other places where / BlockSize.
There was a problem hiding this comment.
Yeah, I forgot that it will only optimize to a bit-shift if it's uint. I should probably change the fields here to be uint.
| } | ||
|
|
||
| private static int GetBlockIndex(int offset) => offset / BlockSize; | ||
| private static int GetOffsetInBlock(int offset) => offset % BlockSize; |
There was a problem hiding this comment.
Cast to uint produces the bit-hack modulo by using and instead of the optimized division.
| private static int GetOffsetInBlock(int offset) => offset % BlockSize; | |
| private static int GetOffsetInBlock(int offset) => (int)((uint)offset % BlockSize); |
|
|
||
| public MultiMemory Slice(int start) | ||
| { | ||
| if (start < 0 || start > _length) |
There was a problem hiding this comment.
| if (start < 0 || start > _length) | |
| if ((uint)start > (uint)_length) |
|
|
||
| public MultiMemory Slice(int start, int length) | ||
| { | ||
| if (start < 0 || length < 0 || start + length > _length) |
There was a problem hiding this comment.
start + length can overflow.
| if (start < 0 || length < 0 || start + length > _length) | |
| if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start)) |
Or tho eke out more perf, one could (on 64-bit) bring it into ulong-domain and compare over there. But I don't think it's worth it.
code
if (Environment.Is64BitProcess)
{
if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
{
throw new IndexOutOfRangeException();
}
}
else
{
if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
{
throw new IndexOutOfRangeException();
}
}
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated
Show resolved
Hide resolved
|
Lots of stream conformance failures... I didn't see these locally. Will investigate. I'm going to factor out the span compare change into a separate PR, so we can get that in and confirm that's not an issue here. |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@stephentoub Just a side question - I see more and more API's using ArrayPool.Shared. And my question is: aren't we going to get pool drained quickly this way (considering user code too)? Or are there any changes to Shared pool implementation? Afaik this pool is not endless if I may call it like that |
|
I use the term "drained", but it's actually a bit misleading, in that it suggests there's a fixed number of buffers that are available in the pool and if they're gone they're gone and the pool is no longer effective... that's not actually how it works. Today each size bucket in the pool has a max number of buffers that it can store, but they're not pre-generated: if you go to the pool and there isn't a buffer available, it manufactures a new one for you. It's on returning a buffer to the pool that it might get thrown away if the relevant bucket has already reached its threshold. And this is important because it means you have can way, way more buffers of a given size "rented" at any one time than the bucket's threshold suggests, and as long as they're not all returned to live in the pool at the same time, many of them won't be thrown away: the pool ends up just being a brief hand-off point between one component adding a buffer and another one coming along and taking one. Strange analogy, but kind of like a bathroom at a restaurant: you don't need one bathroom per customer, rather you can be feeding many, many more customers than you have bathrooms, and you only need as many bathrooms as you want to enable customers to use them at the same time. Which is to say, yes, it's possible that as more and more components use the ArrayPool we'll need to update its thresholds and behaviors to accomodate the demand, but we've not yet seen it to be a problem. #12800 tracks revisiting some of this. |
|
@stephentoub I see. Thanks for the detailed explanation. I've just been concerned that at some point excessive use of array pool might become an "expensive new" in unexpected ways. |
geoffkizer
force-pushed
the
multiarraybuffer
branch
from
db7e8d3 to
f5bfb0f
Compare
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ion about sequence differences, and use in StreamConformanceTests
Co-authored-by: Günther Foidl <[email protected]>
geoffkizer
force-pushed
the
multiarraybuffer
branch
from
24ea741 to
01f98a4
Compare
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/ncl Can someone review? |
| // Unlike ArrayBuffer, the buffer itself is managed using 16K blocks which are added/removed to the block list as necessary. | ||
|
|
||
| // [ActiveBuffer] contains the current buffer contents; these bytes will be preserved on any call to TryEnsureAvailableBytesUpToLimit. | ||
| // [AvailableBuffer] contains the available bytes past the end of the current content, |
There was a problem hiding this comment.
Nit: rather than using braces like [ActiveBuffer], how about using single quotes or backticks, e.g. 'ActiveBuffer'? The braces just immediately make me think we're talking about attributes.
| _allocatedEnd = 0; | ||
| _activeStart = 0; | ||
| _availableStart = 0; | ||
| } |
There was a problem hiding this comment.
Nit: it'd be a bit more efficient (and simpler) to do:
public MultiArrayBuffer(int initialBufferSize) : this()
{
// 'initialBufferSize' is ignored for now. Some callers are passing useful info here that we might want to act on in the future.
Debug.Assert(initialBufferSize >= 0);
}
| } | ||
| else | ||
| { | ||
| // We can shift the array down to make enough space |
There was a problem hiding this comment.
Did you consider making the byte[][] circular rather than shifting like this?
There was a problem hiding this comment.
Yes, that seems like a potentially good optimization. I elected not to do it for now.
| { | ||
| uint ustart = (uint)start; | ||
| uint ulength = (uint)length; | ||
| if (ustart > _length || ustart + ulength > _length) |
There was a problem hiding this comment.
Ah, actually, I remember why I did this.
We aren't explicitly checking here if start >= 0, but if it is, this will get caught in the ustart > _length test. EDIT: To be clear, this is because _length is always less than or equal to int.MaxValue.
Unfortunately the second check here isn't quite right, since it doesn't catch a case like start = 5, length = -1, I think it should instead be ulength > _length - ustart and that should correctly catch all invalid cases.
This is also why we don't have asserts here, as you asked about above.
Note also there are asserts in the MultiMemory constructor, so if we miss something here it will get asserted there.
| } | ||
|
|
||
| public MultiMemory Slice(int start) | ||
| { |
There was a problem hiding this comment.
See comments above
| return new MultiMemory(_blocks, _start + ustart, _length - ustart); | ||
| } | ||
|
|
||
| public MultiMemory Slice(int start, int length) |
There was a problem hiding this comment.
Debug.Assert(start >= 0 && length >= 0);?
There was a problem hiding this comment.
See comments above
| throw new ArgumentOutOfRangeException(nameof(destination)); | ||
| } | ||
|
|
||
| for (int blockIndex = 0; blockIndex < BlockCount; blockIndex++) |
There was a problem hiding this comment.
BlockCount looks like it does some computation. Store it into a local rather than recomputing it for each iteration?
| throw new ArgumentOutOfRangeException(nameof(source)); | ||
| } | ||
|
|
||
| for (int blockIndex = 0; blockIndex < BlockCount; blockIndex++) |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
MultiArrayBuffer is a struct that manages a sliding buffer, where bytes be added at the end and removed at the beginning, similar to existing ArrayBuffer. Unlike ArrayBuffer, it manages a list of fixed-size (16K) blocks internally.
This is most useful for scenarios where the buffer may grow large, and thus managing it as a single array is probably not efficient.
MultiArrayBuffer is now used in the HTTP2 response buffer code, as well as in StreamBuffer (used by ConnectedStreams and mock QUIC provider).
(Edit: This got moved to a separate PR, now merged) Also, change stream conformance tests to not use Assert.Equals for buffer comparsion, as it is very, very slow. This reduces the time of outerloop StreamConformanceTests by approximately 99%.
Edit: Also add AssertExtensions.SequenceEqual to provide more useful diff information when sequence comparison fails, and use it in stream conformance tests.
@stephentoub @dotnet/ncl