colencoding: reuse scratch space when key decoding bytes and decimals#70734
colencoding: reuse scratch space when key decoding bytes and decimals#70734craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
416f22b to
a862a52
Compare
|
Okay, this doesn't just work, yet. |
7bed2fe to
5d469ae
Compare
520f480 to
d78f0aa
Compare
|
I think it should now work. RFAL. |
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
pkg/sql/colencoding/key_encoding.go, line 294 at r1 (raw file):
} // Note that since we're about to perform a deep copy, it'll be ok to // return the scratch slice to be reused by the caller.
I don't understand this comment. Since we've already deep-copied, why does it matter that Set deep-copies or not?
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colencoding/key_encoding.go, line 294 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I don't understand this comment. Since we've already deep-copied, why does it matter that
Setdeep-copies or not?
There are two concerns at play: one is about not referencing the memory used by key (to allow for GC to run on the batch response), another one is making sure it is ok to return scratch in such a manner that if scratch is later modified, then vec.Bytes().Get(idx) will still return the correct result. The first concern is addressed by making a deep-copy when decoding, the second concern is addressed automatically by vec.Bytes().Set() which performs the deep copy. The comment is about the second concern.
Do you think it's worth extending the comment? Or maybe removing it to reduce possible confusion?
mgartner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colencoding/key_encoding.go, line 294 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
There are two concerns at play: one is about not referencing the memory used by
key(to allow for GC to run on the batch response), another one is making sure it is ok to returnscratchin such a manner that ifscratchis later modified, thenvec.Bytes().Get(idx)will still return the correct result. The first concern is addressed by making a deep-copy when decoding, the second concern is addressed automatically byvec.Bytes().Set()which performs the deep copy. The comment is about the second concern.Do you think it's worth extending the comment? Or maybe removing it to reduce possible confusion?
Got it. I think maybe just reword it to something like: "Set() performs a deep copy, so it is safe to return the scratch slice to the caller. Any modifications to the scratch slice made by the caller will not affect the value in the vector".
d78f0aa to
78698c7
Compare
When we're key decoding bytes-like columns, we need to use the scratch byte slice to decode into (in case of decimals, we might need the space temporarily). Previously, we would always allocate a new byte slice only to deep copy it later when calling `coldata.Bytes.Set`. This commit teaches the cFetcher and the relevant decoding methods to reuse the same scratch space which should reduce the memory allocations. One notable change is that now when we're calling `DecodeBytesAscending`, we have to make sure to perform a deep copy so that it is safe to reuse the returned value as the scratch space in the future. Release note: None
78698c7 to
d3c354d
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
|
Build succeeded: |
When we're key decoding bytes-like columns, we need to use the scratch
byte slice to decode into (in case of decimals, we might need the space
temporarily). Previously, we would always allocate a new byte slice only
to deep copy it later when calling
coldata.Bytes.Set. This committeaches the cFetcher and the relevant decoding methods to reuse the same
scratch space which should reduce the memory allocations.
One notable change is that now when we're calling
DecodeBytesAscending, we have to make sure to perform a deep copy sothat it is safe to reuse the returned value as the scratch space in the
future.
Release note: None