[Internal] Binary Encoding: Fixes Performance Regression #4884
Conversation
| 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.
NIT - I admit personal opinion - but I find it unreadable when multiple variables are declared in one line.
| int count, totalBytes = 0, offset = (int)this.Position, length = (int)this.Length; | ||
| byte[] bytes = new byte[length]; | ||
|
|
||
| while ((count = this.innerStream.Read(bytes, offset, length - offset)) > 0) |
There was a problem hiding this comment.
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.
| 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.
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.
| /// </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.
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.
| if (offset < 0 | ||
| || count < 0 | ||
| || (buffer.Length - offset) < count | ||
| || this.innerStream.Position == this.innerStream.Length) |
There was a problem hiding this comment.
You can't do this for any Stream. If this.innerStream.CanSeek==false then Position or Lenght will throw NotSupportedException
|
Follow Up PR created to track this work. |
Pull Request Template
Description
Please include a summary of the change and which issue is fixed. Include samples if adding new API, and include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber