-
Notifications
You must be signed in to change notification settings - Fork 2
cralloc: add ScratchBuffer #19
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
base: main
Are you sure you want to change the base?
Conversation
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.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions
cralloc/scratch_buffer.go
line 33 at r1 (raw file):
// // If the receiver is nil, always allocates a new slice. func (sb *ScratchBuffer) Alloc(n int) []byte {
nit: should this be called AllocUnsafe
or something to make it clearer that the lifetime of the allocated buffer is limited?
cralloc/scratch_buffer.go
line 42 at r1 (raw file):
} // Adapted from slices.Grow(). s = append(s[:0], make([]byte, n)...)
is there any reason for this to not just be s = make([]byte, n)
since we already know the slice's existing capacity is insufficient and we don't want to retain any of the existing data within s
?
Add a helper type for reusing a buffer. This simplifies the typical pattern of passing and returning a slice. It is also useful for cases where a function might return either a slice from the scratch buffer or some other aliased slice (for example, `EncodeMVCCValueForExport` in cockroachdb has to return an extra bool so the caller can know if it should update the scratch buffer).
2ad1782
to
30c614f
Compare
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.
TFTR!
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jbowens)
cralloc/scratch_buffer.go
line 42 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
is there any reason for this to not just be
s = make([]byte, n)
since we already know the slice's existing capacity is insufficient and we don't want to retain any of the existing data withins
?
Yeah, so that the slice grows in size according to append()
heuristics (e.g. double in capacity). Otherwise a pattern where the allocation size is slowly increasing causes us to allocate each time. Added a comment.
Note that we are appending to s[:0]
so we won't be copying any of the old data.
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.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @RaduBerinde)
cralloc/scratch_buffer.go
line 42 at r1 (raw file):
Previously, RaduBerinde wrote…
Yeah, so that the slice grows in size according to
append()
heuristics (e.g. double in capacity). Otherwise a pattern where the allocation size is slowly increasing causes us to allocate each time. Added a comment.Note that we are appending to
s[:0]
so we won't be copying any of the old data.
Got it, thanks!
Add a helper type for reusing a buffer. This simplifies the typical
pattern of passing and returning a slice. It is also useful for cases
where a function might return either a slice from the scratch buffer
or some other aliased slice (for example,
EncodeMVCCValueForExport
in cockroachdb has to return an extra bool so the caller can know if
it should update the scratch buffer).
This change is