-
Notifications
You must be signed in to change notification settings - Fork 494
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
[Internal] Binary Encoding: Fixes Performance Regression #4884
base: master
Are you sure you want to change the base?
Changes from all commits
7230e75
86d07c5
7cc5ca5
d243843
61ff215
3adab73
11e41c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,10 @@ | |
namespace Microsoft.Azure.Cosmos.Serializer | ||
{ | ||
using System; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Azure.Cosmos.Json; | ||
using Microsoft.Azure.Documents; | ||
|
||
|
@@ -18,7 +20,7 @@ internal class CosmosBufferedStreamWrapper : Stream | |
/// <summary> | ||
/// The inner stream being wrapped. | ||
/// </summary> | ||
private readonly CloneableStream innerStream; | ||
private readonly Stream innerStream; | ||
|
||
/// <summary> | ||
/// Indicates whether the inner stream should be disposed. | ||
|
@@ -41,7 +43,7 @@ internal class CosmosBufferedStreamWrapper : Stream | |
/// <param name="inputStream">The input stream to wrap.</param> | ||
/// <param name="shouldDisposeInnerStream">Indicates whether the inner stream should be disposed.</param> | ||
public CosmosBufferedStreamWrapper( | ||
CloneableStream inputStream, | ||
Stream inputStream, | ||
bool shouldDisposeInnerStream) | ||
{ | ||
this.innerStream = inputStream ?? throw new ArgumentNullException(nameof(inputStream)); | ||
|
@@ -93,7 +95,33 @@ public override int Read(byte[] buffer, int offset, int count) | |
throw new ArgumentNullException(nameof(buffer)); | ||
} | ||
|
||
return this.innerStream.Read(buffer, offset, count); | ||
if (offset < 0 | ||
|| count < 0 | ||
|| (buffer.Length - offset) < count | ||
|| this.innerStream.Position == this.innerStream.Length) | ||
{ | ||
return 0; | ||
} | ||
|
||
int bytesRead = 0; | ||
if (this.hasReadFirstByte | ||
&& this.innerStream.Position == 1 | ||
&& offset == 0 | ||
&& count > 0) | ||
{ | ||
buffer[0] = this.firstByteBuffer[0]; | ||
bytesRead = 1; | ||
offset++; | ||
count--; | ||
} | ||
|
||
if (count > 0) | ||
{ | ||
int innerBytesRead = this.innerStream.Read(buffer, offset, count); | ||
bytesRead += innerBytesRead; | ||
} | ||
|
||
return bytesRead; | ||
} | ||
|
||
/// <inheritdoc /> | ||
|
@@ -129,11 +157,48 @@ protected override void Dispose(bool disposing) | |
/// </returns> | ||
public byte[] ReadAll() | ||
{ | ||
ArraySegment<byte> byteSegment = this.innerStream.GetBuffer(); | ||
int count, totalBytes = 0, offset = (int)this.Position, length = (int)this.Length; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this.Length is not save when the inner stream is not buffered. And the API contract does not ensure that - it would thorw an exception if the inner stream is not buffered. |
||
byte[] bytes = new byte[length]; | ||
|
||
while ((count = this.innerStream.Read(bytes, offset, length - offset)) > 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation here is functionally incorrect when you first use Stream.Read and subsequently Stream.ReadAll - for example you would copy the first byte again in the buffer for ReadAll - while it has already been processed in that case. |
||
{ | ||
offset += count; | ||
totalBytes += count; | ||
} | ||
|
||
if (this.hasReadFirstByte) | ||
{ | ||
bytes[0] = this.firstByteBuffer[0]; | ||
totalBytes += 1; | ||
} | ||
|
||
return totalBytes > 0 ? bytes : default; | ||
} | ||
|
||
/// <summary> | ||
/// Asynchronously reads all bytes from the current position to the end of the stream. | ||
/// </summary> | ||
/// <returns> | ||
/// A task that represents the asynchronous read operation. The value of the TResult parameter contains a byte array with all the bytes read from the stream, or <c>null</c> if no bytes were read. | ||
/// </returns> | ||
public async Task<byte[]> ReadAllAsync(CancellationToken cancellationToken = default) | ||
{ | ||
int count, totalBytes = 0, offset = (int)this.Position, length = (int)this.Length; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comments in sync PAI. In general I think Stream implementations should implement the async variation and just await it in the sync overload. The perf overhead of doing it even if the asnyc overload completes synchronously is relatively small. |
||
byte[] bytes = new byte[length]; | ||
|
||
while ((count = await this.innerStream.ReadAsync(bytes, offset, length - offset, cancellationToken)) > 0) | ||
{ | ||
offset += count; | ||
totalBytes += count; | ||
} | ||
|
||
if (this.hasReadFirstByte) | ||
{ | ||
bytes[0] = this.firstByteBuffer[0]; | ||
totalBytes += 1; | ||
} | ||
|
||
return byteSegment.Array.Length == byteSegment.Count | ||
? byteSegment.Array | ||
: byteSegment.ToArray(); | ||
return totalBytes > 0 ? bytes : default; | ||
} | ||
|
||
/// <summary> | ||
|
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 - I admit personal opinion - but I find it unreadable when multiple variables are declared in one line.