GODRIVER-3533 Optimize value reader and writer #2022
Conversation
prestonvasquez
requested review from
qingyang-hu and
Copilot
and removed request for
qingyang-hu
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
bson/value_reader.go:50
- [nitpick] The field name 'ra' is ambiguous; consider renaming it to 'readerAt' for improved clarity.
ra io.ReaderAt // The underlying reader
API Change ReportNo changes found! |
| @@ -253,14 +280,28 @@ func (vr *valueReader) appendNextElement(dst []byte) ([]byte, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| buf := make([]byte, length) | |||
| _, err = io.ReadFull(vr.r, buf) | |||
| buf, err := vr.r.Peek(int(length)) | |||
There was a problem hiding this comment.
Peek only allocates once for the bufio's internal buffer, we can borrow views of it with Peek without touching the heap. And append makes the copy.
| @@ -33,6 +33,29 @@ func putValueWriter(vw *valueWriter) { | |||
| } | |||
| } | |||
|
|
|||
| var documentWriterPool = sync.Pool{ | |||
| New: func() interface{} { | |||
| return NewDocumentWriter(nil) | |||
There was a problem hiding this comment.
Can we call newDocumentWriter() because it returns a *valueWriter for syntactic clarity?
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the BSON value writer and reader by introducing sync.Pool usage for pooling document writers and readers, and by updating the element appending logic to use peek and discard. Key changes include:
- Replacing direct allocations in value writer/reader creation with pooled instances.
- Refactoring the reader’s element appending to use bufio.Reader’s Peek and Discard methods.
- Adding safe resource release (e.g. returning pooled readers) in both marshal and unmarshal paths.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| bson/value_writer.go | Introduced a documentWriterPool with helper functions for reuse. |
| bson/value_reader.go | Added pools for bufio.Reader and valueReader with updated reset and release logic. |
| bson/unmarshal.go | Updated to properly release pooled valueReader after unmarshalling. |
| bson/marshal.go | Replaced NewDocumentWriter with pooled value writers via getDocumentWriter. |
GODRIVER-3533
Summary
Use sync.Pool for the value writer and reader when encoding and decoding, respectively. Additionally, use peek and discard when appending elements to the read dst. The danger here occurs when a user finds a way to call Get() more than Put() which would result in a "leak" of reusable objects.
Note: Regressions from v1 will be addressed in GODRIVER-3450. The primary goal of this PR is to add optimizations that were removed in the v2 implementation.
Marshaling Benchmarks
v1:
PR:
Note that we improved in deep BSON test cases but regressed with full BSON test cases. Otherwise the results are fairly similar.
Unmarshaling Benchmarks
v1:
PR:
Background & Motivation
Some optimizations were removed while flatting the bson library during the v2 implementation. Specifically:
bson.valueReaderwith bufio. #1732