-
Notifications
You must be signed in to change notification settings - Fork 350
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
Create test cases and benchmarks for writing large json base64 and string values #2849
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
1 similar comment
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
1 similar comment
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@@ -14,6 +19,11 @@ public class Customer | |||
public int Id { get; set; } | |||
public string Name { get; set; } | |||
public List<string> Emails { get; set; } | |||
public string Bio { get; set; } | |||
|
|||
[JsonConverter(typeof(Base64Converter))] |
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.
Curious why we need a JsonConverter here
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.
JsonSerializer
does not seem to serialize byte[]
fields by default.
{ | ||
using var outputStream = new FileStream(filePath, FileMode.Create, FileAccess.Write, share: FileShare.ReadWrite); | ||
await writer.WritePayloadAsync(data, outputStream, includeRawValues); | ||
//using var outputStream = new FileStream(filePath, FileMode.Create, FileAccess.Write, share: FileShare.ReadWrite); |
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.
Why did you need to comment this statement instead of deleting it?
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.
Oversight. Thanks.
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.
Left a few comments; otherwise LGTM
test/PerformanceTests/SerializationComparisonsTests/JsonWriterBenchmarks/Benchmarks.cs
Outdated
Show resolved
Hide resolved
@@ -65,29 +71,30 @@ public async Task WriteToMemoryAsync() | |||
public async Task WriteToFileAsync() | |||
{ | |||
// multiple writes to increase benchmark duration |
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.
This comment needs to be updated also?
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.
Two writes are still multiple. I think the intent behind the comment still stands.
await WritePayloadAsync(); | ||
await WritePayloadAsync(); | ||
await WritePayloadAsync(data); | ||
await WritePayloadAsync(data); | ||
} | ||
|
||
[Benchmark] | ||
[BenchmarkCategory("RawValues")] | ||
public async Task WriteWithRawValues() | ||
{ | ||
// multiple writes to increase benchmark duration |
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: here to..
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.
@@ -35,6 +35,7 @@ public ODataMessageWriterAsyncPayloadWriter(IEdmModel model, Func<Stream, IOData | |||
public async Task WritePayloadAsync(IEnumerable<Customer> payload, Stream stream, bool includeRawValues) | |||
{ | |||
ODataMessageWriterSettings settings = new ODataMessageWriterSettings(); | |||
settings.EnableMessageStreamDisposal = false; |
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.
why did you add this setting? what does it do?
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.
When this setting is set to true and dispose ODataMessageWriter
, it will dispose the output stream as well. I disabled it because I wanted to use the output stream after finishing the write. For example, in the unit tests for these benchmarks I rewind the MemoryStream
after the writer has completed so that I can read the contents and verify that it wrote the right thing.
…Benchmarks/Benchmarks.cs Co-authored-by: Elizabeth Okerio <elizaokerio@gmail.com>
dac9de0
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
…ring values (OData#2849) * Create test cases for writing large json base64 and string values * Update missing async tests * Add text and binary fields to benchmarks * Remove unnecessary benchmark scenarios * Optimize JsonSerializer converter * Add large value benchmarks * Remove lagging comma * Reduce benchmarks runtime * Reduce benchmarks runtime * Target dotnet 8.0 in benchmarks * Remove unnecessary benchmark scenarios * Optimize benchmarks runtime * largeFields flag in benchmarks * update readme * Update test/PerformanceTests/SerializationComparisonsTests/JsonWriterBenchmarks/Benchmarks.cs Co-authored-by: Elizabeth Okerio <elizaokerio@gmail.com> * Remove obsolete comment --------- Co-authored-by: Elizabeth Okerio <elizaokerio@gmail.com>
#2877) * Create test cases and benchmarks for writing large json base64 and string values (#2849) * Create test cases for writing large json base64 and string values * Update missing async tests * Add text and binary fields to benchmarks * Remove unnecessary benchmark scenarios * Optimize JsonSerializer converter * Add large value benchmarks * Remove lagging comma * Reduce benchmarks runtime * Reduce benchmarks runtime * Target dotnet 8.0 in benchmarks * Remove unnecessary benchmark scenarios * Optimize benchmarks runtime * largeFields flag in benchmarks * update readme * Update test/PerformanceTests/SerializationComparisonsTests/JsonWriterBenchmarks/Benchmarks.cs Co-authored-by: Elizabeth Okerio <elizaokerio@gmail.com> * Remove obsolete comment --------- Co-authored-by: Elizabeth Okerio <elizaokerio@gmail.com> * update changes --------- Co-authored-by: Clément Habinshuti <haby_habbes@live.com>
Issues
This PR partially addresses #2845
Description
It adds test cases that ensures writing large byte arrays and string values with to the JSON writer works correctly. Once merge, this will help verify the optimizations introduced by PR #2846 are correct.
The PR also adds benchmarks with large string and
byte[]
values. I also removed benchmark scenarios that are not necessary, like:JsonWriter
orUtf8JsonWriter
without usingODataMessageWriter
Those benchmark scenarios don't add much value, but they add to the run time of the benchmarks and maintenance overhead.
Checklist (Uncheck if it is not completed)