From ded29be883e888081174a9be88de4ce2618303af Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Aug 2019 15:43:57 -0700 Subject: [PATCH] slice buffer improvements --- .../Internal/SliceBufferSafeHandleTest.cs | 95 ++++++++++++++++++- .../Internal/SliceBufferSafeHandle.cs | 33 +++++-- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/src/csharp/Grpc.Core.Tests/Internal/SliceBufferSafeHandleTest.cs b/src/csharp/Grpc.Core.Tests/Internal/SliceBufferSafeHandleTest.cs index 563187948d2..61c817dafc9 100644 --- a/src/csharp/Grpc.Core.Tests/Internal/SliceBufferSafeHandleTest.cs +++ b/src/csharp/Grpc.Core.Tests/Internal/SliceBufferSafeHandleTest.cs @@ -27,12 +27,99 @@ namespace Grpc.Core.Internal.Tests public class SliceBufferSafeHandleTest { [TestCase] - public void BasicTest() + public void Complete_EmptyBuffer() { - + using (var sliceBuffer = SliceBufferSafeHandle.Create()) + { + sliceBuffer.Complete(); + CollectionAssert.AreEqual(new byte[0], sliceBuffer.ToByteArray()); + } } - // other ideas: - // AdjustTailSpace(0) if previous tail size is 0... (better for SliceBufferSafeHandle) + [TestCase] + public void Complete_TailSizeZero() + { + using (var sliceBuffer = SliceBufferSafeHandle.Create()) + { + var origPayload = GetTestBuffer(10); + origPayload.AsSpan().CopyTo(sliceBuffer.GetSpan(origPayload.Length)); + sliceBuffer.Advance(origPayload.Length); + // call complete where tail space size == 0 + sliceBuffer.Complete(); + CollectionAssert.AreEqual(origPayload, sliceBuffer.ToByteArray()); + } + } + + [TestCase] + public void Complete_TruncateTailSpace() + { + using (var sliceBuffer = SliceBufferSafeHandle.Create()) + { + var origPayload = GetTestBuffer(10); + var dest = sliceBuffer.GetSpan(origPayload.Length + 10); + origPayload.AsSpan().CopyTo(dest); + sliceBuffer.Advance(origPayload.Length); + // call complete where tail space needs to be truncated + sliceBuffer.Complete(); + CollectionAssert.AreEqual(origPayload, sliceBuffer.ToByteArray()); + } + } + + [TestCase] + public void SliceBufferIsReusable() + { + using (var sliceBuffer = SliceBufferSafeHandle.Create()) + { + var origPayload = GetTestBuffer(10); + origPayload.AsSpan().CopyTo(sliceBuffer.GetSpan(origPayload.Length)); + sliceBuffer.Advance(origPayload.Length); + sliceBuffer.Complete(); + CollectionAssert.AreEqual(origPayload, sliceBuffer.ToByteArray()); + + sliceBuffer.Reset(); + + var origPayload2 = GetTestBuffer(20); + origPayload2.AsSpan().CopyTo(sliceBuffer.GetSpan(origPayload2.Length)); + sliceBuffer.Advance(origPayload2.Length); + sliceBuffer.Complete(); + CollectionAssert.AreEqual(origPayload2, sliceBuffer.ToByteArray()); + + sliceBuffer.Reset(); + + CollectionAssert.AreEqual(new byte[0], sliceBuffer.ToByteArray()); + } + } + + [TestCase] + public void SliceBuffer_SizeHintZero() + { + using (var sliceBuffer = SliceBufferSafeHandle.Create()) + { + var destSpan = sliceBuffer.GetSpan(0); + Assert.IsTrue(destSpan.Length > 0); // some non-zero size memory is made available + + sliceBuffer.Reset(); + + var destMemory = sliceBuffer.GetMemory(0); + Assert.IsTrue(destMemory.Length > 0); + } + } + + // Advance() - multiple chunks... + + // other TODOS: + + // -- provide a null instance of SliceBufferSafeHandle + //-- order of UnsafeSerialize in AsyncCall methods... + + private byte[] GetTestBuffer(int length) + { + var testBuffer = new byte[length]; + for (int i = 0; i < testBuffer.Length; i++) + { + testBuffer[i] = (byte) i; + } + return testBuffer; + } } } diff --git a/src/csharp/Grpc.Core/Internal/SliceBufferSafeHandle.cs b/src/csharp/Grpc.Core/Internal/SliceBufferSafeHandle.cs index 0ce37317f1a..77c14f1d9fb 100644 --- a/src/csharp/Grpc.Core/Internal/SliceBufferSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/SliceBufferSafeHandle.cs @@ -31,6 +31,7 @@ namespace Grpc.Core.Internal /// internal class SliceBufferSafeHandle : SafeHandleZeroIsInvalid, IBufferWriter { + const int DefaultTailSpaceSize = 4096; // default buffer to allocate if no size hint is provided static readonly NativeMethods Native = NativeMethods.Get(); static readonly ILogger Logger = GrpcEnvironment.Logger.ForType(); @@ -72,7 +73,7 @@ namespace Grpc.Core.Internal // Use GetSpan when possible for better efficiency. public Memory GetMemory(int sizeHint = 0) { - GetSpan(sizeHint); + EnsureBufferSpace(sizeHint); if (memoryManagerLazy == null) { memoryManagerLazy = new SliceMemoryManager(); @@ -84,13 +85,7 @@ namespace Grpc.Core.Internal // provides access to the "tail space" of this buffer. public unsafe Span GetSpan(int sizeHint = 0) { - GrpcPreconditions.CheckArgument(sizeHint >= 0); - if (tailSpaceLen < sizeHint) - { - // TODO: should we ignore the hint sometimes when - // available tail space is close enough to the sizeHint? - AdjustTailSpace(sizeHint); - } + EnsureBufferSpace(sizeHint); return new Span(tailSpacePtr.ToPointer(), tailSpaceLen); } @@ -135,7 +130,27 @@ namespace Grpc.Core.Internal return result; } - // Gets data of server_rpc_new completion. + private void EnsureBufferSpace(int sizeHint) + { + GrpcPreconditions.CheckArgument(sizeHint >= 0); + if (sizeHint == 0) + { + // if no hint is provided, keep the available space within some "reasonable" boundaries. + // This is quite a naive approach which could use some fine-tuning, but currently in most case we know + // the required buffer size in advance anyway, so this approach seems good enough for now. + if (tailSpaceLen < DefaultTailSpaceSize /2 ) + { + AdjustTailSpace(DefaultTailSpaceSize); + } + } + else if (tailSpaceLen < sizeHint) + { + // if hint is provided, always make sure we provide at least that much space + AdjustTailSpace(sizeHint); + } + } + + // make sure there's exactly requestedSize bytes of continguous buffer space at the end of this slice buffer private void AdjustTailSpace(int requestedSize) { GrpcPreconditions.CheckArgument(requestedSize >= 0);