Repository navigation
Conversation
| { | ||
| _jsonPayloadSequence = GetSequence(_jsonPayload, SegmentSize); | ||
| } | ||
| _jsonPayloadSequence = GetSequence(_jsonPayload, 100); |
There was a problem hiding this comment.
you've mentioned 4096 would be a better test
There was a problem hiding this comment.
We should re-consider the test, sure, but I am primarily making a cosmetic change here.
|
|
||
| [Benchmark] | ||
| public void Utf8JsonReaderCommentParsing() | ||
| public void EmptyLoop() |
There was a problem hiding this comment.
Maybe. However, we don't really "consume". I would think consume to mean observing each token (either via reader.ValueSpan or reader.GetString/GetInt/etc.)
There was a problem hiding this comment.
for me, reading is consuming (also matches naming convention used in the product). on the other hand for me empty loop means:
while (true) {}
| { | ||
| var state = new JsonReaderState(options: new JsonReaderOptions { CommentHandling = CommentHandling }); | ||
| Utf8JsonReader reader = MultiSegment ? | ||
| Utf8JsonReader reader = !SingleSegment ? |
There was a problem hiding this comment.
You can also swap expressions and remove !
| { | ||
| [BenchmarkCategory(Categories.CoreFX, Categories.JSON)] | ||
| public partial class Utf8JsonReaderCommentsTests | ||
| public partial class Utf8JsonReader_Comments |
There was a problem hiding this comment.
Maybe would be better to put Utf8JsonReader somewhere in the namespace and making a class name just CommentsTests
There was a problem hiding this comment.
I thought about that, but then the test namespace doesn't follow the API namespace structure. I believe I was told that the pattern is <API name space>.Tests.
|
I think it was my antivirus before which was causing the weird perf results 🤦♂️ |
|
@ahsonkhan @adamsitnik @krwq is this ready? |
|
nothing blocking on my side, would be nice if we've addressed the feedback though. @ahsonkhan, have time to address it? |
|
I'll try to do an update today before I leave. Thanks for the reminder. |
|
@ahsonkhan ping |
4 participants
From #543 (comment)
Primarily to make the output more reasonable to read and to remove the redundant
Utf8JsonReaderwithin the fully qualified test name (especially for the result comparer view - dotnet/corefx#38036 (comment)):System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsingAlso, trying to be consistent with the other perf test format.
cc @krwq, @adamsitnik