colexec: batch allocate datums for projectTupleOp#74724
colexec: batch allocate datums for projectTupleOp#74724craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Nice find! I think this change is very good,
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/sql/colexec/tuple_proj_op.go, line 73 at r1 (raw file):
} t.allocator.PerformOperation([]coldata.Vec{projVec}, func() {
So actually we will perform the memory accounting for all these tuples - t.allocator.PerformOperation measures how much space projVec is taking up before the anonymous function, how much after the function, and adjusts the memory account accordingly. In this case we assume that old tuples in projVec have been discarded (we're overwriting the references in projVec itself right now, and if the caller wanted to hold on to the old tuples, it must have copied them somewhere and adjusted its memory account).
My comment about DatumAlloc was applicable to the row-by-row engine where we don't have an easy way to track the lifetime of the datums and don't have helpers like PerformOperation to perform the accounting.
pkg/sql/colexec/tuple_proj_op.go, line 76 at r1 (raw file):
// Preallocate the tuples and their underlying datums in a contiguous // slice to reduce allocations. tuples := make([]tree.DTuple, n)
I started thinking whether we can reuse any of these allocations between iterations of tupleProjOp.Next, and I think we cannot. We definitely cannot reuse tuples[i] because all datums are assumed to be immutable. datums[::] backs each tuple, so if we were to change it, we'd mutate the already returned tuple - also no bueno. Same goes for tuples.
7ad3e53 to
7a3ca8d
Compare
ajwerner
left a comment
There was a problem hiding this comment.
I realized we don't need two loops. It helps another on the runtime.
Reviewable status:
pkg/sql/colexec/tuple_proj_op.go, line 73 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
So actually we will perform the memory accounting for all these tuples -
t.allocator.PerformOperationmeasures how much spaceprojVecis taking up before the anonymous function, how much after the function, and adjusts the memory account accordingly. In this case we assume that old tuples inprojVechave been discarded (we're overwriting the references inprojVecitself right now, and if the caller wanted to hold on to the old tuples, it must have copied them somewhere and adjusted its memory account).My comment about
DatumAllocwas applicable to the row-by-row engine where we don't have an easy way to track the lifetime of the datums and don't have helpers likePerformOperationto perform the accounting.
Ack.
pkg/sql/colexec/tuple_proj_op.go, line 76 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I started thinking whether we can reuse any of these allocations between iterations of
tupleProjOp.Next, and I think we cannot. We definitely cannot reusetuples[i]because all datums are assumed to be immutable.datums[::]backs each tuple, so if we were to change it, we'd mutate the already returned tuple - also no bueno. Same goes fortuples.
Ack.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
-- commits, line 4 at r2:
nit: s/amound/amount/.
This has a profound impact on the amount of garbage generated by the delivery query in TPC-C. ``` name old time/op new time/op delta TPCC/mix=delivery=1-16 38.0ms ± 2% 35.8ms ± 1% -5.76% (p=0.000 n=9+8) name old alloc/op new alloc/op delta TPCC/mix=delivery=1-16 8.17MB ± 1% 7.97MB ± 1% -2.36% (p=0.000 n=9+10) name old allocs/op new allocs/op delta TPCC/mix=delivery=1-16 80.2k ± 0% 20.3k ± 1% -74.65% (p=0.000 n=10+9) ``` Leveraged cockroachdb#74443 to find this. Release note: None
7a3ca8d to
57732b1
Compare
ajwerner
left a comment
There was a problem hiding this comment.
TFTR!
bors r=yuzefovich
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/amound/amount/.
Done.
|
Build succeeded: |
This has a profound impact on the amound of garbage generated by the delivery
query in TPC-C.
Leveraged #74443 to find this.
Release note: None