-
Notifications
You must be signed in to change notification settings - Fork 909
GODRIVER-3533 Optimize value reader and writer #2022
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
70c4fbb
20145ad
e0f81a0
8848df3
3afc6f8
ecb98ce
d6bdec4
2797876
c97c5cc
81e684f
5eba4a7
31cfdad
d00dd6c
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 | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ import ( | |||||||||||||||||||||
"fmt" | ||||||||||||||||||||||
"io" | ||||||||||||||||||||||
"math" | ||||||||||||||||||||||
"sync" | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
var _ ValueReader = &valueReader{} | ||||||||||||||||||||||
|
@@ -29,6 +30,20 @@ type vrState struct { | |||||||||||||||||||||
end int64 | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
var bufioReaderPool = sync.Pool{ | ||||||||||||||||||||||
New: func() interface{} { | ||||||||||||||||||||||
return bufio.NewReader(nil) | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
var vrPool = sync.Pool{ | ||||||||||||||||||||||
New: func() interface{} { | ||||||||||||||||||||||
return &valueReader{ | ||||||||||||||||||||||
stack: make([]vrState, 1, 5), | ||||||||||||||||||||||
} | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// valueReader is for reading BSON values. | ||||||||||||||||||||||
type valueReader struct { | ||||||||||||||||||||||
r *bufio.Reader | ||||||||||||||||||||||
|
@@ -38,6 +53,33 @@ type valueReader struct { | |||||||||||||||||||||
frame int64 | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
func getDocumentReader(r io.Reader) *valueReader { | ||||||||||||||||||||||
vr := vrPool.Get().(*valueReader) | ||||||||||||||||||||||
|
||||||||||||||||||||||
vr.offset = 0 | ||||||||||||||||||||||
vr.frame = 0 | ||||||||||||||||||||||
|
||||||||||||||||||||||
vr.stack = vr.stack[:1] | ||||||||||||||||||||||
vr.stack[0] = vrState{mode: mTopLevel} | ||||||||||||||||||||||
|
||||||||||||||||||||||
br := bufioReaderPool.Get().(*bufio.Reader) | ||||||||||||||||||||||
br.Reset(r) | ||||||||||||||||||||||
vr.r = br | ||||||||||||||||||||||
|
||||||||||||||||||||||
return vr | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
func putDocumentReader(vr *valueReader) { | ||||||||||||||||||||||
if vr == nil { | ||||||||||||||||||||||
return | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
bufioReaderPool.Put(vr.r) | ||||||||||||||||||||||
vr.r = nil | ||||||||||||||||||||||
|
||||||||||||||||||||||
vrPool.Put(vr) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// NewDocumentReader returns a ValueReader using b for the underlying BSON | ||||||||||||||||||||||
// representation. | ||||||||||||||||||||||
func NewDocumentReader(r io.Reader) ValueReader { | ||||||||||||||||||||||
|
@@ -253,14 +295,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
if err == bufio.ErrBufferFull { | ||||||||||||||||||||||
temp := make([]byte, length) | ||||||||||||||||||||||
if _, err = io.ReadFull(vr.r, temp); err != nil { | ||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
dst = append(dst, temp...) | ||||||||||||||||||||||
vr.offset += int64(len(temp)) | ||||||||||||||||||||||
return dst, nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
Comment on lines
+301
to
+309
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. [nitpick] Consider refactoring the error handling for bufio.ErrBufferFull in appendNextElement to streamline the code and reduce duplication in the fallback path.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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 suggestion is a hallucination., vr.readAndAppend() is not part of the valueReader API. |
||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
dst = append(dst, buf...) | ||||||||||||||||||||||
vr.offset += int64(len(buf)) | ||||||||||||||||||||||
return dst, err | ||||||||||||||||||||||
if _, err = vr.r.Discard(int(length)); err != nil { | ||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
vr.offset += int64(length) | ||||||||||||||||||||||
return dst, nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
func (vr *valueReader) readValueBytes(dst []byte) (Type, []byte, error) { | ||||||||||||||||||||||
|
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.
Consider resetting the bufio.Reader (for example, by calling Reset(nil)) before returning it to the pool to clear its internal buffer and help prevent unintended memory retention.
Copilot uses AI. Check for mistakes.
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 a dangerous change that could cause OOB errors, e.g: