-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Improve ArrayBufferWriter re-alloc perf when size is > int.MaxSize / 2 #42950
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
Changes from all commits
73ee4c7
d56d39a
8262b30
ac3f296
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 |
|---|---|---|
|
|
@@ -1004,7 +1004,10 @@ private void Grow(int requiredSize) | |
| { | ||
| Debug.Assert(_arrayBufferWriter != null); | ||
|
|
||
| _memory = _arrayBufferWriter.GetMemory(checked(BytesPending + sizeHint)); | ||
| int needed = BytesPending + sizeHint; | ||
|
Contributor
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. This was changed to keep the OOM exception semantics (instead of OverflowException). There are no effective perf differences since the overhead of the new Note there are other cases in STJ that also use
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.
Add a test, if one doesn't already exist?
Contributor
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 the existing test that was modified in this PR was failing due to this (not throwing OOM as expected). |
||
| JsonHelpers.ValidateInt32MaxArrayLength((uint)needed); | ||
|
|
||
| _memory = _arrayBufferWriter.GetMemory(needed); | ||
|
|
||
| Debug.Assert(_memory.Length >= sizeHint); | ||
| } | ||
|
|
||
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.
Can we just use
Array.MaxArrayLength, so we don't have this constant in multiple places?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 file is compiled in NS2.0 assemblies, so it needs to have a local copy. I agree it would be nice to have an API for this - reactivating #31366
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.
Will it matter on full framework where MaxArrayLength might be smaller (I believe not due to how the buffer grows, but I thought I'd double check)?
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.
Right, it is not easy to find the maximum array length in .NET Framework. It depends on
gcAllowVeryLargeObjectsconfiguration setting. It just means that this code may throw OOM on .NET Framework even when it could have been avoided. I think that is fine.