colexec: integrate flat, compact decimal datums #74590
Conversation
nvanbenschoten
requested review from
adityamaru and
stevendanna
and removed request for
a team
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/bigInt3
branch
from
851bdcd to
0c4dc2f
Compare
There was a problem hiding this comment.
Major kudos for working on this! Thanks for all of the detailed descriptions of the changes.
General thoughts:
- should we prohibit the usage of
math/big.Intand force everyone to useapd.BigInt? - similar to the second commit, I think we can remove
execgen.OverloadHelper.TmpDecs. Curious whether you want to include the corresponding change into this PR. Also, leaving aTODO(yuzefovich)for this also sounds good to me.
Reviewed 116 of 119 files at r1, 8 of 8 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @jordanlewis, @nvanbenschoten, and @stevendanna)
-- commits, line 9 at r1:
nit: no release note.
go.mod, line 35 at r1 (raw file):
github.com/cenkalti/backoff v2.2.1+incompatible github.com/client9/misspell v0.3.4 github.com/cockroachdb/apd/v3 v3.0.0-20220107230737-bd134301dd9a
I've never updated the vendored stuff myself, so it'd be good if someone else took a look at that.
pkg/sql/colmem/allocator.go, line 398 at r3 (raw file):
// decimalEstimate is our guess for how much space a single apd.Decimal element // will take up. const decimalEstimate = 50
Let's adjust this estimate to be the size of the zero value of apd.Decimal.
pkg/sql/sem/builtins/aggregate_builtins.go, line 2007 at r3 (raw file):
a.sxy.Set(&a.tmpSxy) size := int64(a.n.Sign() +
These should be Size, not Sign :)
pkg/util/duration/duration.go, line 302 at r3 (raw file):
} // FromBigInt converts an apb.BigInt number of nanoseconds to a duration. Inverse
nit: s/apb/apd/.
74341: sql/catalog: restore fast-path in FullIndexColumnIDs r=ajwerner a=nvanbenschoten This commit restores a [fast-path](c9e116e#diff-19625608f4a6e23e6fe0818f3a621e716615765cb338d18fe34b43f0a535f06dL140) in `FullIndexColumnIDs` which was lost in c9e116e. The fast-path avoided the allocation of a `ColumnID` slice and a `IndexDescriptor_Direction` slice in `FullIndexColumnIDs` when given a unique index. In such cases, these slices are already stored on the `IndexDescriptor`. ``` name old time/op new time/op delta KV/Scan/SQL/rows=1-10 94.9µs ±10% 94.9µs ± 8% ~ (p=0.739 n=10+10) name old alloc/op new alloc/op delta KV/Scan/SQL/rows=1-10 20.1kB ± 0% 20.1kB ± 1% ~ (p=0.424 n=10+10) name old allocs/op new allocs/op delta KV/Scan/SQL/rows=1-10 245 ± 0% 241 ± 0% -1.63% (p=0.000 n=10+8) ``` ---- This is part of a collection of assorted micro-optimizations: - #74336 - #74337 - #74338 - #74339 - #74340 - #74341 - #74342 - #74343 - #74344 - #74345 - #74346 - #74347 - #74348 Combined, these changes have the following effect on end-to-end SQL query performance: ``` name old time/op new time/op delta KV/Scan/SQL/rows=1-10 94.4µs ±10% 92.3µs ±11% -2.20% (p=0.000 n=93+93) KV/Scan/SQL/rows=10-10 102µs ±10% 99µs ±10% -2.16% (p=0.000 n=94+94) KV/Update/SQL/rows=10-10 378µs ±15% 370µs ±11% -2.04% (p=0.003 n=95+91) KV/Insert/SQL/rows=1-10 133µs ±14% 132µs ±12% ~ (p=0.738 n=95+93) KV/Insert/SQL/rows=10-10 197µs ±14% 196µs ±13% ~ (p=0.902 n=95+94) KV/Update/SQL/rows=1-10 186µs ±14% 185µs ±14% ~ (p=0.351 n=94+93) KV/Delete/SQL/rows=1-10 132µs ±13% 132µs ±14% ~ (p=0.473 n=94+94) KV/Delete/SQL/rows=10-10 254µs ±16% 250µs ±16% ~ (p=0.086 n=100+99) name old alloc/op new alloc/op delta KV/Scan/SQL/rows=1-10 20.1kB ± 0% 19.1kB ± 1% -4.91% (p=0.000 n=96+96) KV/Scan/SQL/rows=10-10 21.7kB ± 0% 20.7kB ± 1% -4.61% (p=0.000 n=96+97) KV/Delete/SQL/rows=10-10 64.0kB ± 3% 63.7kB ± 3% -0.55% (p=0.000 n=100+100) KV/Update/SQL/rows=1-10 45.8kB ± 1% 45.5kB ± 1% -0.55% (p=0.000 n=97+98) KV/Update/SQL/rows=10-10 105kB ± 1% 105kB ± 1% -0.10% (p=0.008 n=97+98) KV/Delete/SQL/rows=1-10 40.8kB ± 0% 40.7kB ± 0% -0.08% (p=0.001 n=95+96) KV/Insert/SQL/rows=1-10 37.4kB ± 1% 37.4kB ± 0% ~ (p=0.698 n=97+96) KV/Insert/SQL/rows=10-10 76.4kB ± 1% 76.4kB ± 0% ~ (p=0.822 n=99+98) name old allocs/op new allocs/op delta KV/Scan/SQL/rows=1-10 245 ± 0% 217 ± 0% -11.43% (p=0.000 n=95+92) KV/Scan/SQL/rows=10-10 280 ± 0% 252 ± 0% -10.11% (p=0.000 n=75+97) KV/Delete/SQL/rows=10-10 478 ± 0% 459 ± 0% -4.04% (p=0.000 n=94+97) KV/Delete/SQL/rows=1-10 297 ± 1% 287 ± 1% -3.34% (p=0.000 n=97+97) KV/Update/SQL/rows=1-10 459 ± 0% 444 ± 0% -3.27% (p=0.000 n=97+97) KV/Insert/SQL/rows=1-10 291 ± 0% 286 ± 0% -1.72% (p=0.000 n=82+86) KV/Update/SQL/rows=10-10 763 ± 1% 750 ± 1% -1.68% (p=0.000 n=96+98) KV/Insert/SQL/rows=10-10 489 ± 0% 484 ± 0% -1.03% (p=0.000 n=98+98) ``` 74355: kv: protect Replica's lastToReplica and lastFromReplica fields with raftMu r=nvanbenschoten a=nvanbenschoten This commit moves the Replica's lastToReplica and lastFromReplica from under the `Replica.mu` mutex to the `Replica.raftMu` mutex. These are strictly Raft-specific pieces of state, so we don't need fine-grained locking around them. As a reward, we don't need to grab the `Replica.mu` exclusively (or at all) when setting the fields in `Store.withReplicaForRequest`. The locking in `setLastReplicaDescriptors` showed up in a mutex profile under a write-heavy workload. It was responsible for **3.44%** of mutex wait time. Grabbing the mutex was probably also slowing down request processing, as the exclusive lock acquisition had to wait for read locks to be dropped. <img width="1584" alt="Screen Shot 2021-12-30 at 9 45 08 PM" src="https://user-images.githubusercontent.com/5438456/147800455-8da74dfd-5fd3-4831-818c-7e3c65763435.png"> 74592: coldata: operate on Nulls value, not reference r=yuzefovich a=nvanbenschoten This commit changes `col.Vec.SetNulls` to accept a `Nulls` struct by value instead of by pointer. This lets us avoid a heap allocation on each call to `Nulls.Or`. We saw this in the "after" heap profiles in #74590, which looked like: <img width="1749" alt="Screen Shot 2022-01-07 at 7 17 32 PM" src="https://user-images.githubusercontent.com/5438456/148624263-777a6d93-4df7-40da-84a3-18d5e47ab633.png"> ``` File: cockroach Type: alloc_objects Time: Jan 8, 2022 at 12:17am (UTC) Showing nodes accounting for 5943494, 100% of 5943494 total ----------------------------------------------------------+------------- flat flat% sum% cum cum% calls calls% + context ----------------------------------------------------------+------------- 843873 48.47% | github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj.projPlusDecimalDecimalOp.Next.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go:3938 823389 47.29% | github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj.projPlusInt64Int64Op.Next.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go:5732 73736 4.24% | github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj.projPlusInt64DecimalOp.Next.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go:5870 1740998 29.29% 29.29% 1740998 29.29% | github.com/cockroachdb/cockroach/pkg/col/coldata.(*Nulls).Or /go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/nulls.go:350 ----------------------------------------------------------+------------- 819219 49.50% | github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj.projPlusInt64Int64Op.Next.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go:5732 704530 42.57% | github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj.projPlusDecimalDecimalOp.Next.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go:3938 131076 7.92% | github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj.projPlusInt64DecimalOp.Next.func1 /go/src/github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go:5870 1654825 27.84% 57.14% 1654825 27.84% | github.com/cockroachdb/cockroach/pkg/col/coldata.(*Nulls).Or /go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/nulls.go:348 ----------------------------------------------------------+------------- ``` This PR eliminates one of these two heap allocations. Co-authored-by: Nathan VanBenschoten <[email protected]>
nvanbenschoten
force-pushed
the
nvanbenschoten/bigInt3
branch
5 times, most recently
from
7d50825 to
c8fe6c3
Compare
This commit picks up the following changes to `cockroachdb/apd`: - cockroachdb/apd#103 - cockroachdb/apd#104 - cockroachdb/apd#107 - cockroachdb/apd#108 - cockroachdb/apd#109 - cockroachdb/apd#110 - cockroachdb/apd#111 Release note (performance improvement): The memory representation of DECIMAL datums has been optimized to save space, avoid heap allocations, and eliminate indirection. This increases the speed of DECIMAL arithmetic and aggregation by up to 20% on large data sets.
`apd.Decimal` can now be entirely stack allocated during arithmetic, so there's no longer any need for this. With cockroachdb/apd#104, this does not introduce any new heap allocations: ``` ➜ (cd pkg/sql/sem/tree && goescape . | grep moved | wc -l) 328 ```
nvanbenschoten
force-pushed
the
nvanbenschoten/bigInt3
branch
from
c8fe6c3 to
1c431f0
Compare
There was a problem hiding this comment.
TFTRs!
should we prohibit the usage of math/big.Int and force everyone to use apd.BigInt?
I considered this, but there are a few valid reasons to continue using big.Int in a few edge cases. For instance, we still need to use it when interacting with big.Rat and we still need to use it when interacting with external libraries like github.com/jackc/pgtype. So for now, I think I'll leave it as an option, but we can crack down on it later.
similar to the second commit, I think we can remove execgen.OverloadHelper.TmpDecs. Curious whether you want to include the corresponding change into this PR. Also, leaving a TODO(yuzefovich) for this also sounds good to me.
This is a good idea. I think I'll leave it out of this PR to avoid expanding its scope any further, but I added a TODO to pkg/sql/colexec/execgen/overloads_util.go.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: no release note.
Done.
pkg/sql/colmem/allocator.go, line 398 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Let's adjust this estimate to be the size of the zero value of
apd.Decimal.
Done.
pkg/sql/sem/builtins/aggregate_builtins.go, line 2007 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
These should be
Size, notSign:)
🤦♂️ Done.
pkg/util/duration/duration.go, line 302 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/apb/apd/.
Done.
|
I posted some benchmark results from |
There was a problem hiding this comment.
Awesome! 🚀
Reviewed 82 of 82 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
Replaces #74369 and #57593.
This PR picks up the following changes to
cockroachdb/apd:Release note (performance improvement): The memory representation of DECIMAL datums has been optimized to save space, avoid heap allocations, and eliminate indirection. This increases the speed of DECIMAL arithmetic and aggregation by up to 20% on large data sets.
At a high-level, those changes implement the "compact memory representation" for Decimals described in cockroachdb/apd#102 (comment) and later implemented in cockroachdb/apd#103.
Compared to the approach on master, the approach in cockroachdb/apd#103 is a) faster, b) avoids indirection + heap allocation, c) smaller.
Compared to the alternate approach in cockroachdb/apd#102, the approach in cockroachdb/apd#103 is a) faster for most operations, b) more usable because values can be safely copied, c) half the memory size (32 bytes per
Decimal, vs. 64).The memory representation of the Decimal struct in this approach looks like:
With a two-word inline array, any value that would fit in a 128-bit integer (i.e. decimals with a scale-adjusted absolute value up to 2^128 - 1) fit in
_inline. The indirection through_inneris only used for values larger than this.Before this change, the memory representation of the
Decimalstruct looked like:Performance impact
Speedup on TPC-DS dataset
The TPC-DS dataset is full of decimal columns, so it's a good playground to test this change. Unfortunately, the variance in the runtime performance of the TPC-DS queries themselves is high (many queries varied by 30-40% per attempt), so it was hard to get signal out of them. Instead, I imported the TPC-DS dataset with a scale factor of 10 and ran some custom aggregation queries against the largest table (
web_sales, row count = 7,197,566):Queries
Here's the difference in runtime of these three queries before and after this change on an
n2-standard-4instance:Heap allocation reduction in TPC-DS
Part of the reason for this speedup was that it significantly reduces heap allocations because most decimal values are stored inline. We can see this in q3 from above. Before the change, a heap profile looks like:
After the change, a heap profile looks like:
(the dominant source of heap allocations is now
coldata.(*Nulls).Or. #74592 should help here)Heap allocation reduction in TPC-E
On the read-only portion of the TPC-E (77% of the full workload, in terms of txn mix), this change has a significant impact on total heap allocations. Before the change,
math/big.nat.makewas responsible for 51.07% of total heap allocations:After the change,
math/big.nat.makeis responsible for only 1.1% of total heap allocations:That equates to roughly a 50% reduction in heap allocations.
Microbenchmarks