Skip to content

Expand DefaultBufferSize remarks #3146

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

Merged
merged 6 commits into from
Oct 3, 2019
Merged

Expand DefaultBufferSize remarks #3146

merged 6 commits into from
Oct 3, 2019

Conversation

tdykstra
Copy link
Contributor

@tdykstra tdykstra commented Sep 9, 2019

@tdykstra tdykstra requested a review from ahsonkhan September 9, 2019 15:15
@mairaw mairaw added this to the September 2019 milestone Sep 9, 2019
Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text-wise looks good. Just left some suggestions to add the thousand separator. @ahsonkhan should review for accuracy.

tdykstra and others added 2 commits September 9, 2019 09:57
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com>
@mairaw mairaw added doc-enhancement Improve the current content 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Sep 9, 2019
The default buffer size, in bytes, is 16,384.
For most workloads, the default size is a reasonable amount of JSON to buffer while reading from a stream or writing to a stream.
That is, it performs well without creating objects in the Large Object Heap for the Garbage Collector (GC) to track.
We recommend that you leave this value unchanged unless changing it makes an observable difference in performance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @steveharter - please review

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Co-Authored-By: Ahson Khan <ahson_ahmedk@yahoo.com>
@tdykstra tdykstra requested a review from steveharter September 9, 2019 23:50
@mairaw
Copy link
Contributor

mairaw commented Sep 13, 2019

@steveharter is this good to merge?

@mairaw
Copy link
Contributor

mairaw commented Oct 2, 2019

Should we just merge this one or should we wait for @steveharter's approval?

For most workloads, the default size is a reasonable amount of JSON to buffer while reading from a stream or writing to a stream.
That is, it performs well without creating objects on the Large Object Heap for the Garbage Collector (GC) to track.
We recommend that you leave this value unchanged unless changing it makes an observable difference in performance.
For example, if your JSON is always in small payloads, you could run tests with the default buffer size at 4,096 to find out if performance improves significantly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider adding something like this as well.

Conversely, if your JSON is known to be really large and in memory (such as returned as string or UTF-8 bytes rather than writing to a stream), setting the default buffer size to a large upper bound could help improve performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think decreasing it will improve performance in any normal scenario unless the available memory is extremely low (since 16K is not very much). However @ahsonkhan suggestion of increasing it in non-streaming scenarios is a known benefit for JSON sizes that are known to be large.

@ahsonkhan
Copy link
Contributor

Should we just merge this one

I think it looks good to merge.

@mairaw mairaw removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Oct 3, 2019
@tdykstra tdykstra merged commit 6b6ca59 into master Oct 3, 2019
@tdykstra tdykstra deleted the tdykstra-patch-1 branch October 3, 2019 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 doc-enhancement Improve the current content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants