colrpc: add memory accounting for temporary data in inbox/outbox#67577
colrpc: add memory accounting for temporary data in inbox/outbox#67577craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Previously, we forgot to update the allocator once we have deserialized the batch in the inbox (i.e. we would register only the original allocation, upon the batch's creation). This could result in under-accounting for variable length types, and it is now fixed. Release note: None
|
This change is |
|
I'm kinda debating whether it's worth adding the memory accounting to the inbox/outbox in the row-by-row engine since it'll require more plumbing and seems less useful. Thoughts? |
cucaroach
left a comment
There was a problem hiding this comment.
Don't know enough to say but sounds plausible.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @michae2)
cucaroach
left a comment
There was a problem hiding this comment.
Reviewable status:
michae2
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:
pkg/sql/colflow/colrpc/inbox.go, line 354 at r2 (raw file):
// At this point, we have lost all references to the serialized bytes, // so we update the allocator accordingly. i.allocator.AdjustMemoryUsage(-numSerializedBytes)
At this point, don't we still have roughly the same amount of data in i.scratch.data? Seems like that doesn't get released until the next call to Next. Maybe instead of counting m.Data which is so short-lived we should count i.scratch.data?
pkg/sql/colflow/colrpc/outbox.go, line 118 at r2 (raw file):
o.scratch.buf = nil o.scratch.msg = nil o.allocator.AdjustMemoryUsage(-o.allocator.Used())
nit: maybe Allocator should have a Close method that does this?
pkg/sql/colflow/colrpc/outbox.go, line 305 at r2 (raw file):
if newBufCap := o.scratch.buf.Cap(); newBufCap > oldBufCap { o.allocator.AdjustMemoryUsage(int64(newBufCap - oldBufCap)) }
nit: do we need this to be conditional?
8f012bc to
58819a3
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colflow/colrpc/inbox.go, line 354 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
At this point, don't we still have roughly the same amount of data in
i.scratch.data? Seems like that doesn't get released until the next call toNext. Maybe instead of countingm.Datawhich is so short-lived we should counti.scratch.data?
Actually ArrowToBatch nils out fields in i.scratch.data, so I believe that all references to the memory are truly lost and at this point we only have the reference to data inside of i.scratch.b which is accounted for. Updated the comment.
pkg/sql/colflow/colrpc/outbox.go, line 118 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
nit: maybe Allocator should have a Close method that does this?
Hm, I think it might be confusing because BoundAccount has Close method and Allocator.Close currently shouldn't call it because the account is closed on the flow cleanup. I guess my expectation is if we were to introduce Allocator.Close, then it must call Allocator.acc.Close too which would require changes to other places.
pkg/sql/colflow/colrpc/outbox.go, line 305 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
if newBufCap := o.scratch.buf.Cap(); newBufCap > oldBufCap { o.allocator.AdjustMemoryUsage(int64(newBufCap - oldBufCap)) }nit: do we need this to be conditional?
No, refactored.
michae2
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colflow/colrpc/inbox.go, line 354 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Actually
ArrowToBatchnils out fields ini.scratch.data, so I believe that all references to the memory are truly lost and at this point we only have the reference to data inside ofi.scratch.bwhich is accounted for. Updated the comment.
Ah, great! Never mind, thank you!
pkg/sql/colflow/colrpc/outbox.go, line 118 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Hm, I think it might be confusing because
BoundAccounthasClosemethod andAllocator.Closecurrently shouldn't call it because the account is closed on the flow cleanup. I guess my expectation is if we were to introduceAllocator.Close, then it must callAllocator.acc.Closetoo which would require changes to other places.
Ok, that's fine.
Previously, we were missing the memory accounting for the temporary usage of serialized/deserialized bytes in the inbox/outbox pair. The worst offense was that in the outbox we reuse the same scratch Bytes buffer that is never truncated and can only increase in capacity throughout the lifetime of the outbox, yet it wasn't accounted for. This commit fixes those omissions. Release note: None
58819a3 to
32e05a9
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Added a minor adjustment to the outbox closure.
TFTRs!
bors r+
Reviewable status:
|
Build failed (retrying...): |
cucaroach
left a comment
There was a problem hiding this comment.
Reviewed 2 of 3 files at r3, 1 of 1 files at r4.
Reviewable status:
|
Build succeeded: |
|
blathers backport 21.1 |
This comment has been minimized.
This comment has been minimized.
|
blathers backport 21.1 |
colrpc: fix memory accounting in inbox a bit
Previously, we forgot to update the allocator once we have deserialized
the batch in the inbox (i.e. we would register only the original
allocation, upon the batch's creation). This could result in
under-accounting for variable length types, and it is now fixed.
Release note: None
colrpc: add memory accounting for temporary data in inbox/outbox
Previously, we were missing the memory accounting for the temporary
usage of serialized/deserialized bytes in the inbox/outbox pair. The
worst offense was that in the outbox we reuse the same scratch Bytes
buffer that is never truncated and can only increase in capacity
throughout the lifetime of the outbox, yet it wasn't accounted for. This
commit fixes those omissions.
Fixes: #67051.
Release note: None