-
Notifications
You must be signed in to change notification settings - Fork 918
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 1 commit
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,13 +30,30 @@ 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 | ||||||||
offset int64 | ||||||||
r *bufio.Reader | ||||||||
ra io.ReaderAt // The underlying reader | ||||||||
|
||||||||
stack []vrState | ||||||||
frame int64 | ||||||||
poolReader bool // Set if r is from bufioReaderPool | ||||||||
|
||||||||
offset int64 | ||||||||
stack []vrState | ||||||||
frame int64 | ||||||||
} | ||||||||
|
||||||||
// NewDocumentReader returns a ValueReader using b for the underlying BSON | ||||||||
|
@@ -57,14 +75,33 @@ func newValueReader(t Type, r io.Reader) ValueReader { | |||||||
} | ||||||||
|
||||||||
func newDocumentReader(r io.Reader) *valueReader { | ||||||||
stack := make([]vrState, 1, 5) | ||||||||
stack[0] = vrState{ | ||||||||
mode: mTopLevel, | ||||||||
} | ||||||||
return &valueReader{ | ||||||||
r: bufio.NewReader(r), | ||||||||
stack: stack, | ||||||||
vr := vrPool.Get().(*valueReader) | ||||||||
|
||||||||
vr.offset = 0 | ||||||||
vr.frame = 0 | ||||||||
vr.poolReader = true | ||||||||
|
||||||||
vr.stack = vr.stack[:1] | ||||||||
vr.stack[0].mode = mTopLevel | ||||||||
|
||||||||
|
||||||||
br := bufioReaderPool.Get().(*bufio.Reader) | ||||||||
br.Reset(r) | ||||||||
vr.r = br | ||||||||
|
||||||||
return vr | ||||||||
} | ||||||||
|
||||||||
func releaseDocumentReader(vr *valueReader) { | ||||||||
qingyang-hu marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
if !vr.poolReader { | ||||||||
return | ||||||||
} | ||||||||
|
||||||||
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. 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.
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 is a dangerous change that could cause OOB errors, e.g:
|
||||||||
bufioReaderPool.Put(vr.r) | ||||||||
vr.r = nil | ||||||||
vr.poolReader = false | ||||||||
|
||||||||
vr.ra = nil | ||||||||
vrPool.Put(vr) | ||||||||
} | ||||||||
|
||||||||
func (vr *valueReader) advanceFrame() { | ||||||||
|
@@ -253,14 +290,18 @@ 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 { | ||||||||
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.
newDocumentReader()
is also called wherereleaseDocumentReader()
is not called. It seems we will have to reversenewDocumentReader()
and add areset()
forvalueReader
like what we did forvalueWriter
.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.
Good catch!