Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GODRIVER-2914 bsoncodec/bsonrw: eliminate encoding allocations #1323
GODRIVER-2914 bsoncodec/bsonrw: eliminate encoding allocations #1323
Changes from all commits
f9c485a
4beb9fe
80680c8
603bb4a
6c41835
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Optional: Consider adding a comment about why we can check individual bytes rather than runes.
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.
Real reason is that
'\x00'
is a byte (it equals0
not00000000
) and since it is less that MaxRune (0x80
) it just callsIndexByte
- this change just omits an unnecessary function call (and one that can't currently be inlined because its complexity score exceeds what is allowed for inlining).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.
I think we're both saying the same thing.
0x80
(hex) ==10000000
(binary). I can update the comment with a reference to the corresponding code instrings.ContainsRune
for additional clarity.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.
Ah, I thought you meant
10000000
as hex or a 32bit rune here since that's what IndexRune takes.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 is normally best practice, but the Go standard library's "encoding/json" package does not perform this check when returning
*encodeState
to its pool (encoding/json/encode.go#L268-L279). I assume the reasoning here is that a program that encodes large structs typically does so frequently. An example here would be a user that stores large documents in their Mongodb database - most often many of the blobs will be of similar size so if some are large - then many of the other documents will be large as well.If we do keep this logic then we should probably only check the capacity since I'm not sure what the check for
sw.Cap()/2 < sw.Len()
buys us and may lead to thrashing when dealing with a mix of small and large BSON documents.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.
Yep, the current "encoding/json" library doesn't do any pooled buffer size checking, which is actually the subject of golang/go#27735. We got the idea to measure buffer utilization as a simple way to prevent holding onto a bunch of mostly unused memory from that issue.
Under ideal usage patterns where messages are all the same size, the pooled memory will be mostly utilized. The pathological case is when there are mostly small messages with a few very large messages. That can cause the pooled buffers to grow very large, holding onto a lot of memory that is underutilized by the mostly small messages. That issue happened for some customers after we added buffer pooling to the operation logic and lead to GODRIVER-2677. The Go "encoding/json" library can currently also suffer from the same memory pooling problem.
One possible solution is to keep "buckets" of buffers of different size, so large buffers are used for large messages and small buffers are used for small messages. However, the implementation for that is relatively complex, so we decided to go with the simpler solution of setting a maximum buffer size and minimum utilization inspired by this comment. The trade off is holding onto less unused memory but possibly allocating memory more often for large messages.
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.
Thank you for pointing out that issue. I think the easiest safe thing to do would be to increase the size limit of the buffers that we'll pool to something like 32mb (2x the max size of BSON docs or even 24mb) and remove the check to see if half the capacity is used. The motivation here is that due to how
bytes.Buffer
grows a Buffer with a length over 8mb could easily have a cap greater than 16mb.We should also check if the size is equal to or less than whatever limit we pick.. Also, this isn't that big of a deal since the difference here will likely only be an allocation or two.