opt: optimize plan gist encoding and decoding #138641

mgartner · 2025-01-08T15:01:16Z

opt/exec: split plan gist factory and decoder

The plan gist factory is no longer used to decode plan gists. The
planGistDecoder has been introduced to fulfill this role.

Release note: None

opt/exec: use `bytes.Reader` in `planGistDecoder`

planGistDecoder now uses a bytes.Reader instead of a bytes.Buffer.
Writing bytes to a bytes.Buffer is done with the Write method which
copies the bytes into the buffer, allocating more space if necessary. A
bytes.Reader can read over an existing slice of bytes with the Reset
method without copying or allocating.

Release note: None

util/base64: add `Encoder`

The base64.Encoder utility has been added for encoding byte slice
streams into a base64 string. It is adapted from the standard library's
base64 encoder returned by base64.NewEncoder with some simplifications
and an API that helps eliminate unnecessary allocations.

Release note: None

util: break dependence on util/intsets

The util package no longer imports util/intsets. This will allow
util/intsets to import util in future commits without creating an
import cycle.

Release note: None

opt/exec: use `base64.Encoder` for plan gist encoding

base64.Encoder is now used in PlanGistFactory to simplify the logic
and reduce allocations.

Release note: None

opt/exec: encode plan gist intsets directly as base64

The intsets.(*Fast).EncodeBase64 method has been added and it is now
used to base64 encode intsets directly in plan gists. This avoids the
intermediate step of encoding the intset to a temporary buffer before
base64 encoding it.

Release note: None

sql: embed `explain.PlanGistFactory` in `optPlanningCtx`

A reusable explain.PlanGistFactory is now embedded in optPlanningCtx
to avoid re-allocation of the factory for each query.

Release note: None

opt/exec: remove `explain.NewPlanGistFactory`

explain.NewPlanGistFactory has been replaced by
(*PlanGistFactory).Init.

Epic: None

Release note: None

cockroach-teamcity · 2025-01-08T15:01:58Z

This change is

mgartner · 2025-01-08T15:08:04Z

name                                        old time/op    new time/op    delta
EndToEnd/kv-read/vectorize=on/Prepared-16      145µs ± 1%     139µs ± 1%  -4.34%  (p=0.000 n=20+20)
EndToEnd/kv-read/vectorize=off/Prepared-16     139µs ± 1%     133µs ± 2%  -3.98%  (p=0.000 n=20+20)
EndToEnd/kv-read/vectorize=on/Simple-16        213µs ± 1%     206µs ± 1%  -3.32%  (p=0.000 n=19+20)
EndToEnd/kv-read/vectorize=off/Simple-16       207µs ± 1%     200µs ± 1%  -3.01%  (p=0.000 n=20+14)

name                                        old alloc/op   new alloc/op   delta
EndToEnd/kv-read/vectorize=on/Prepared-16     20.5kB ± 0%    20.3kB ± 0%  -1.09%  (p=0.000 n=20+20)
EndToEnd/kv-read/vectorize=off/Prepared-16    21.7kB ± 0%    21.5kB ± 0%  -0.83%  (p=0.000 n=20+19)
EndToEnd/kv-read/vectorize=off/Simple-16      33.8kB ± 0%    33.6kB ± 0%  -0.63%  (p=0.000 n=19+20)
EndToEnd/kv-read/vectorize=on/Simple-16       33.4kB ± 0%    33.2kB ± 0%  -0.52%  (p=0.000 n=20+18)

name                                        old allocs/op  new allocs/op  delta
EndToEnd/kv-read/vectorize=on/Prepared-16        186 ± 0%       183 ± 0%  -1.11%  (p=0.000 n=20+20)
EndToEnd/kv-read/vectorize=off/Prepared-16       178 ± 0%       176 ± 0%  -0.96%  (p=0.000 n=17+20)
EndToEnd/kv-read/vectorize=on/Simple-16          280 ± 0%       277 ± 0%  -0.89%  (p=0.000 n=20+18)
EndToEnd/kv-read/vectorize=off/Simple-16         267 ± 0%       265 ± 0%  -0.75%  (p=0.000 n=18+18)

mgartner · 2025-01-08T15:20:20Z

pkg/util/base64/base64.go

+}
+
+// Write encodes p in an internal buffer.
+func (e *Encoder) Write(p []byte) {


This code was adapted from: https://cs.opensource.google/go/go/+/refs/tags/go1.23.4:src/encoding/base64/base64.go;l=212-286

yuzefovich

Nice!

Reviewed 1 of 2 files at r1, 17 of 17 files at r9, 1 of 1 files at r10, 4 of 4 files at r11, 2 of 2 files at r12, 2 of 2 files at r13, 5 of 5 files at r14, 2 of 2 files at r15, 4 of 4 files at r16, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)

pkg/util/base64/base64.go line 6 at r11 (raw file):

// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with

nit: I think we use a different license header now.

pkg/util/base64/base64.go line 35 at r11 (raw file):

}

// Init initliazes an Encoder with the given base64.Encoding.

nit: s/initliazes/initializes/.

pkg/util/base64/base64.go line 75 at r11 (raw file):

	copy(e.buf[:], p)
	e.nbuf = int8(len(p))
	return

nit: no need for this return since the function now doesn't return anything.

mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @yuzefovich)

pkg/util/base64/base64.go line 6 at r11 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think we use a different license header now.

Good catch, done.

pkg/util/base64/base64.go line 35 at r11 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/initliazes/initializes/.

Done.

pkg/util/base64/base64.go line 75 at r11 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: no need for this return since the function now doesn't return anything.

Done.

mgartner · 2025-01-08T19:08:29Z

Ran some sysbench benchmarks out of curiosity:

name                                         old time/op    new time/op    delta
Sysbench/SQL/1node_local/oltp_read_only-16     2.72ms ± 1%    2.74ms ± 1%  +0.66%  (p=0.001 n=19+20)
Sysbench/SQL/1node_local/oltp_read_write-16    4.69ms ± 1%    4.72ms ± 1%  +0.80%  (p=0.000 n=20+20)

name                                         old alloc/op   new alloc/op   delta
Sysbench/SQL/1node_local/oltp_read_only-16      836kB ± 0%     834kB ± 0%  -0.26%  (p=0.000 n=19+17)
Sysbench/SQL/1node_local/oltp_read_write-16    1.27MB ±12%    1.36MB ±12%    ~     (p=0.201 n=20+20)

name                                         old allocs/op  new allocs/op  delta
Sysbench/SQL/1node_local/oltp_read_only-16      3.90k ± 1%     3.88k ± 1%  -0.64%  (p=0.000 n=19+19)
Sysbench/SQL/1node_local/oltp_read_write-16     6.73k ± 1%     6.71k ± 1%  -0.34%  (p=0.000 n=20+19)

The plan gist factory is no longer used to decode plan gists. The `planGistDecoder` has been introduced to fulfill this role. Release note: None

`planGistDecoder` now uses a `bytes.Reader` instead of a `bytes.Buffer`. Writing bytes to a `bytes.Buffer` is done with the `Write` method which copies the bytes into the buffer, allocating more space if necessary. A `bytes.Reader` can read over an existing slice of bytes with the `Reset` method without copying or allocating. Release note: None

mgartner · 2025-01-08T22:30:59Z

Sysbench with sql.metrics.statement_details.enabled = false and sql.metrics.transaction_details.enabled = false:

name                                         old time/op    new time/op    delta
Sysbench/SQL/1node_local/oltp_read_only-16     2.69ms ± 1%    2.69ms ± 1%    ~     (p=1.000 n=19+20)
Sysbench/SQL/1node_local/oltp_read_write-16    4.62ms ± 2%    4.62ms ± 2%    ~     (p=0.478 n=20+20)

name                                         old alloc/op   new alloc/op   delta
Sysbench/SQL/1node_local/oltp_read_only-16      804kB ± 0%     802kB ± 0%  -0.29%  (p=0.000 n=18+20)
Sysbench/SQL/1node_local/oltp_read_write-16    1.25MB ± 9%    1.28MB ±11%    ~     (p=0.620 n=20+20)

name                                         old allocs/op  new allocs/op  delta
Sysbench/SQL/1node_local/oltp_read_only-16      3.79k ± 0%     3.77k ± 0%  -0.54%  (p=0.000 n=18+20)
Sysbench/SQL/1node_local/oltp_read_write-16     6.51k ± 1%     6.48k ± 1%  -0.35%  (p=0.003 n=20+20)

The `base64.Encoder` utility has been added for encoding byte slice streams into a base64 string. It is adapted from the standard library's base64 encoder returned by `base64.NewEncoder` with some simplifications and an API that helps eliminate unnecessary allocations. Release note: None

The `util` package no longer imports `util/intsets`. This will allow `util/intsets` to import `util` in future commits without creating an import cycle. Release note: None

`base64.Encoder` is now used in `PlanGistFactory` to simplify the logic and reduce allocations. Release note: None

The `intsets.(*Fast).EncodeBase64` method has been added and it is now used to base64 encode intsets directly in plan gists. This avoids the intermediate step of encoding the intset to a temporary buffer before base64 encoding it. Release note: None

A reusable `explain.PlanGistFactory` is now embedded in `optPlanningCtx` to avoid re-allocation of the factory for each query. Release note: None

`explain.NewPlanGistFactory` has been replaced by `(*PlanGistFactory).Init`. Release note: None

mgartner · 2025-01-09T13:45:30Z

TFTR!

bors r+

craig · 2025-01-09T14:54:41Z

Build succeeded:

PR cockroachdb#138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None

137805: sql/row: use Put instead of CPut when updating value of secondary index r=yuzefovich,stevendanna a=michae2 **sql/row: use Put instead of CPut when updating value of secondary index** When an UPDATE statement changes the value but not the key of a secondary index (e.g. an update to the stored columns of a secondary index) we need to write a new version of the secondary index KV with the new value. We were using a CPutAllowingIfNotExists to do this, which verified that if the KV existed, the expected value was the value before update. But there's no need for this verification. We have other mechanisms to detect a write-write conflict with any other transaction that could have changed the value concurrently. We can simply use a Put to overwrite the previous value. This also matches what we do for the primary index when the PK doesn't change. Epic: None Release note: None --- **sql: change CPutAllowingIfNotExists with nil expValue to CPut** CPutAllowingIfNotExists with empty expValue is equivalent to CPut with empty expValue, so change a few spots to use regular CPut. This almost gets rid of CPutAllowingIfNotExists entirely, but there is at least one spot in backfill (introduced in #138707) that needs to allow for both a non-empty expValue and non-existence of the KV. Also drop "(expecting does not exist)" from CPut tracing, as CPut with empty expValue is now overwhelmingly the most common use of CPut. And this matches the tracing in #138707. Epic: None Release note: None 138243: changefeedccl: fix PTS test r=stevendanna a=asg0451 Fix failing TestPTSRecordProtectsTargetsAndSystemTables test Fixes: #135639 Fixes: #138066 Fixes: #137885 Fixes: #137505 Fixes: #136396 Fixes: #135805 Fixes: #135639 Release note: None 138696: sql: add CHECK EXTERNAL CONNECTION command r=kev-cao a=kev-cao This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None 138740: opt/bench: fix benchmark regression r=mgartner a=mgartner #### opt/bench: fix benchmark regression PR #138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None #### util/base64: use `bytes.Buffer` instead of `strings.Builder` in `Encoder` For our purposes in base64-encoding plan gists, using `bytes.Buffer` in `Encoder` causes fewer allocations, presumably because of a more aggressive growth algorithm. Epic: None Release note: None 138877: opt: reduce allocations when filtering histogram buckets r=mgartner a=mgartner `cat.HistogramBuckets` are now returned and passed by value in `getFilteredBucket` and `(*Histogram).addBucket`, respectively, eliminating some heap allocations. Also, two allocations when building spans from buckets via the `spanBuilder` have been combined into one. The new `(*spanBuilder).init` method simplifies the API by no longer requiring that prefix datums are passed to every invocation of `makeSpanFromBucket`. This also reduces redundant copying of the prefix. Epic: None Release note: None 139029: sql/logictest: disable column family mutations in some cases r=mgartner a=mgartner Random column family mutations are now disabled for `CREATE TABLE` statements with unique, hash-sharded indexes. This prevents the AST from being reserialized with a `UNIQUE` constraint with invalid options, instead of the original `UNIQUE INDEX`. See #65929 and #107398. Epic: None Release note: None 139039: ccl/serverccl: revise `TestTenantVars` cpu time checks r=xinhaoz a=xinhaoz Previously, this test verified that a portion of the test's user cpu time would be less than or equal to the entire tenant user cpu time up to that point. This check is flaky because there's no guarantee that the inactive tenant's cpu time will surpass the test cpu time. We now simply verify that the test cpu times are greater than or equal to the tenant metrics. The test was likely passing before 331596c because the reported tenant cpu time was accounting for the sql server prestart. A tenant's user cpu metrics are tracked from the time the `_status/load` endpoint is registered, and the commit above moved the router setup to occur just after the prestart. Epic: none Fixes: #119329 Release note: None Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: Miles Frankel <[email protected]> Co-authored-by: Kevin Cao <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]>

138740: opt/bench: fix benchmark regression r=mgartner a=mgartner #### opt/bench: fix benchmark regression PR #138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None #### util/base64: use `bytes.Buffer` instead of `strings.Builder` in `Encoder` For our purposes in base64-encoding plan gists, using `bytes.Buffer` in `Encoder` causes fewer allocations, presumably because of a more aggressive growth algorithm. Epic: None Release note: None 139070: spanconfigkvsubscriberccl: run expensive tests in heavy pool r=rafiss a=rafiss We have seen this timeout under race over the past few weeks, so this patch gives the test more resources under race/deadlock. informs: #138032 Release note: None 139074: authors: add Manu Gomez to authors r=InManuBytes a=InManuBytes Epic: None Release note: None Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Manu Gomez <[email protected]>

PR cockroachdb#138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None

mgartner requested a review from a team January 8, 2025 15:01

mgartner requested a review from a team as a code owner January 8, 2025 15:01

mgartner requested review from DrewKimball and removed request for a team January 8, 2025 15:01

mgartner added the o-perf-efficiency Related to performance efficiency label Jan 8, 2025

mgartner commented Jan 8, 2025

View reviewed changes

mgartner force-pushed the plan-gist-cleanup branch 2 times, most recently from 6b539d3 to b350644 Compare January 8, 2025 15:57

yuzefovich approved these changes Jan 8, 2025

View reviewed changes

mgartner force-pushed the plan-gist-cleanup branch from b350644 to a49c063 Compare January 8, 2025 18:27

mgartner commented Jan 8, 2025

View reviewed changes

mgartner added 2 commits January 8, 2025 14:10

opt/exec: split plan gist factory and decoder

318eaf1

The plan gist factory is no longer used to decode plan gists. The `planGistDecoder` has been introduced to fulfill this role. Release note: None

mgartner force-pushed the plan-gist-cleanup branch from a49c063 to f9ee7de Compare January 8, 2025 22:08

mgartner added 6 commits January 8, 2025 17:52

util: break dependence on util/intsets

2984fea

The `util` package no longer imports `util/intsets`. This will allow `util/intsets` to import `util` in future commits without creating an import cycle. Release note: None

opt/exec: use base64.Encoder for plan gist encoding

64e7013

`base64.Encoder` is now used in `PlanGistFactory` to simplify the logic and reduce allocations. Release note: None

sql: embed explain.PlanGistFactory in optPlanningCtx

f2be108

A reusable `explain.PlanGistFactory` is now embedded in `optPlanningCtx` to avoid re-allocation of the factory for each query. Release note: None

opt/exec: remove explain.NewPlanGistFactory

29999dc

`explain.NewPlanGistFactory` has been replaced by `(*PlanGistFactory).Init`. Release note: None

mgartner force-pushed the plan-gist-cleanup branch from f9ee7de to 29999dc Compare January 8, 2025 22:52

craig bot merged commit ffaab21 into cockroachdb:master Jan 9, 2025
22 checks passed

mgartner deleted the plan-gist-cleanup branch January 9, 2025 15:06

mgartner mentioned this pull request Jan 9, 2025

opt/bench: fix benchmark regression #138740

Merged

mohini-crl mentioned this pull request Jan 17, 2025

[Replicated] opt/bench: fix benchmark regression mohini-crl/cockroach#166

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: optimize plan gist encoding and decoding #138641

opt: optimize plan gist encoding and decoding #138641

mgartner commented Jan 8, 2025 •

edited

Loading

cockroach-teamcity commented Jan 8, 2025

mgartner commented Jan 8, 2025

mgartner Jan 8, 2025

yuzefovich left a comment

mgartner left a comment

mgartner commented Jan 8, 2025

mgartner commented Jan 8, 2025

mgartner commented Jan 9, 2025

craig bot commented Jan 9, 2025

opt: optimize plan gist encoding and decoding #138641

opt: optimize plan gist encoding and decoding #138641

Conversation

mgartner commented Jan 8, 2025 • edited Loading

opt/exec: split plan gist factory and decoder

opt/exec: use bytes.Reader in planGistDecoder

util/base64: add Encoder

util: break dependence on util/intsets

opt/exec: use base64.Encoder for plan gist encoding

opt/exec: encode plan gist intsets directly as base64

sql: embed explain.PlanGistFactory in optPlanningCtx

opt/exec: remove explain.NewPlanGistFactory

cockroach-teamcity commented Jan 8, 2025

mgartner commented Jan 8, 2025

mgartner Jan 8, 2025

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

mgartner commented Jan 8, 2025

mgartner commented Jan 8, 2025

mgartner commented Jan 9, 2025

craig bot commented Jan 9, 2025

mgartner commented Jan 8, 2025 •

edited

Loading

opt/exec: use `bytes.Reader` in `planGistDecoder`

util/base64: add `Encoder`

opt/exec: use `base64.Encoder` for plan gist encoding

sql: embed `explain.PlanGistFactory` in `optPlanningCtx`

opt/exec: remove `explain.NewPlanGistFactory`