Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GODRIVER-2914 bsoncodec/bsonrw: eliminate encoding allocations #1323

Merged
merged 5 commits into from
Sep 12, 2023
Merged

GODRIVER-2914 bsoncodec/bsonrw: eliminate encoding allocations #1323

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9c485a
bsoncodec/bsonrw: eliminate encoding allocations
charlievieth Jul 7, 2023
4beb9fe
Code review changes.
matthewdale Sep 5, 2023
80680c8
Replace reflect.Pointer with reflect.Ptr
matthewdale Sep 5, 2023
603bb4a
Improve comment in isValidCString.
matthewdale Sep 6, 2023
6c41835
Merge branch 'master' into cev/bson-allocs
qingyang-hu Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bson/bsonrw/value_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ func (vr *valueReader) ReadValue() (ValueReader, error) {
return nil, ErrEOA
}

if err := vr.consumeCString(); err != nil {
if err := vr.skipCString(); err != nil {
return nil, err
}

Expand Down Expand Up @@ -793,7 +793,7 @@ func (vr *valueReader) readByte() (byte, error) {
return vr.d[vr.offset-1], nil
}

func (vr *valueReader) consumeCString() error {
func (vr *valueReader) skipCString() error {
idx := bytes.IndexByte(vr.d[vr.offset:], 0x00)
if idx < 0 {
return io.EOF
Expand Down
7 changes: 6 additions & 1 deletion bson/bsonrw/value_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,6 @@ func (vw *valueWriter) writeElementHeader(t bsontype.Type, destination mode, cal
}
vw.appendHeader(t, key)
case mValue:
// TODO: Do this with a cache of the first 1000 or so array keys.
vw.appendIntHeader(t, frame.arrkey)
default:
modes := []mode{mElement, mValue}
Expand Down Expand Up @@ -611,6 +610,12 @@ func (vw *valueWriter) writeLength() error {
}

func isValidCString(cs string) bool {
// Disallow the zero byte in a cstring because the zero byte is used as the
// terminating character. It's safe to check bytes instead of runes because
// all multibyte UTF-8 code points start with "11xxxxxx" or "10xxxxxx", so
// "00000000" will never be part of a multibyte UTF-8 code point.
//
// See https://en.wikipedia.org/wiki/UTF-8#Encoding for details.
return strings.IndexByte(cs, 0) == -1
Copy link
Collaborator

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.

Suggested change
return strings.IndexByte(cs, 0) == -1
// Disallow the zero byte in a cstring because the zero byte is used as the
// terminating character. It's safe to check bytes instead of runes because
// all multibyte UTF-8 code points start with "11xxxxxx" or "10xxxxxx", so
// "00000000" will never be part of a multibyte UTF-8 code point.
return strings.IndexByte(cs, 0) == -1

Copy link
Contributor Author

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 equals 0 not 00000000) and since it is less that MaxRune (0x80) it just calls IndexByte - 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).

Copy link
Collaborator

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 in strings.ContainsRune for additional clarity.

Copy link
Contributor Author

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.

}

Expand Down
19 changes: 18 additions & 1 deletion bson/marshal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,24 @@ var bufPool = sync.Pool{
// See [Encoder] for more examples.
func MarshalAppendWithContext(ec bsoncodec.EncodeContext, dst []byte, val interface{}) ([]byte, error) {
sw := bufPool.Get().(*bytes.Buffer)
defer bufPool.Put(sw)
defer func() {
// Proper usage of a sync.Pool requires each entry to have approximately
// the same memory cost. To obtain this property when the stored type
// contains a variably-sized buffer, we add a hard limit on the maximum
// buffer to place back in the pool. We limit the size to 16MiB because
// that's the maximum wire message size supported by any current MongoDB
// server.
//
// Comment based on
// https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/fmt/print.go;l=147
//
// Recycle byte slices that are smaller than 16MiB and at least half
// occupied.
if sw.Cap() < 16*1024*1024 && sw.Cap()/2 < sw.Len() {
bufPool.Put(sw)
}
}()

Copy link
Contributor Author

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.

Copy link
Collaborator

@matthewdale matthewdale Sep 5, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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.

sw.Reset()
vw := bvwPool.Get(sw)
defer bvwPool.Put(vw)
Expand Down
Loading