-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Add read ahead logic for streams. #38039
Changes from all commits
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 |
|---|---|---|
|
|
@@ -145,5 +145,10 @@ private string GetPropertyName(in ReadStackFrame frame) | |
|
|
||
| return propertyName; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Bytes consumed in the current loop | ||
| /// </summary> | ||
| public long BytesConsumed; | ||
|
Contributor
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. Something more descriptive since it is only for the read-ahead. Like
Member
Author
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. We always use it, even when we aren't reading ahead. We had to track it separately because of the Skip logic. I've started a discussion with @ahsonkhan about whether or not we can move this back to the reader as there is currently no way to peek ahead without separately tracking consumed.
JeremyKuhne marked this conversation as resolved.
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.IO; | ||
| using System.Linq; | ||
| using Xunit; | ||
|
|
||
|
|
@@ -145,5 +146,32 @@ public void Verify() | |
| Assert.Equal("Hello", Array[3].ToString()); | ||
| } | ||
| } | ||
|
|
||
| [Theory, | ||
| InlineData(5), | ||
| InlineData(10), | ||
| InlineData(20), | ||
| InlineData(1024)] | ||
| public void ReadJsonElementFromStream(int defaultBufferSize) | ||
|
Contributor
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. Did you verify this actually causes the new code to be hit? The issue is that although the buffer size is specified, we grab from the arraypool which typically will have larger blocks (e.g. 4K).
Member
Author
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. Yes, it does fail without this. The smallest ArrayPool bucket is 16 bytes, so what we see here is 16, 32, and 1024 in reality. |
||
| { | ||
| // Streams need to read ahead when they hit objects or arrays that are assigned to JsonElement or object. | ||
|
|
||
| byte[] data = Encoding.UTF8.GetBytes(@"{""Data"":[1,true,{""City"":""MyCity""},null,""foo""]}"); | ||
|
JeremyKuhne marked this conversation as resolved.
|
||
| MemoryStream stream = new MemoryStream(data); | ||
| JsonElement obj = JsonSerializer.ReadAsync<JsonElement>(stream, new JsonSerializerOptions { DefaultBufferSize = defaultBufferSize }).Result; | ||
|
|
||
| data = Encoding.UTF8.GetBytes(@"[1,true,{""City"":""MyCity""},null,""foo""]"); | ||
| stream = new MemoryStream(data); | ||
| obj = JsonSerializer.ReadAsync<JsonElement>(stream, new JsonSerializerOptions { DefaultBufferSize = defaultBufferSize }).Result; | ||
|
|
||
| // Ensure we fail with incomplete data | ||
| data = Encoding.UTF8.GetBytes(@"{""Data"":[1,true,{""City"":""MyCity""},null,""foo""]"); | ||
| stream = new MemoryStream(data); | ||
| Assert.Throws<JsonException>(() => JsonSerializer.ReadAsync<JsonElement>(stream, new JsonSerializerOptions { DefaultBufferSize = defaultBufferSize }).Result); | ||
|
|
||
| data = Encoding.UTF8.GetBytes(@"[1,true,{""City"":""MyCity""},null,""foo"""); | ||
| stream = new MemoryStream(data); | ||
| Assert.Throws<JsonException>(() => JsonSerializer.ReadAsync<JsonElement>(stream, new JsonSerializerOptions { DefaultBufferSize = defaultBufferSize }).Result); | ||
| } | ||
| } | ||
| } | ||
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.
Ideally we only need to do this when we have a custom converter or JsonElement (including the overflow property bag). However, we can optimize that later.
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 extra seeking is only needed when we don't have the final block and we're reading an object/array into a property value, which I think may not be particularly common? I think we'll have to be somewhat reactionary here when we see real user scenarios that hit this. Not sure how efficiently we can evaluate the serialized object for "simpleness".