Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
…achdb#116876

115454: engineccl: Use crypto/subtle.XORBytes r=bdarnell a=bdarnell

This function (new in Go 1.20) is faster than our hand-written xor loop. This is good for a nearly 15% increase in encryption throughput (for standard intel builds. 5% in fips mode, 20% on Apple M2)

Release note (performance improvement): Improved performance of encryption-at-rest.
Epic: None

```
bdarnell@gceworker-bdarnell-fips:~/go/src/github.com/cockroachdb/cockroach$ ~/go/bin/benchstat before.bench after-xor.bench goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                    │ before.bench │           after-xor.bench           │
                                                    │    sec/op    │   sec/op     vs base                │
FileCipherStream/fips=false/key=128/block=16/-24       64.63n ± 0%   57.87n ± 2%  -10.45% (p=0.000 n=10)
FileCipherStream/fips=false/key=128/block=1024/-24     2.221µ ± 0%   1.884µ ± 0%  -15.17% (p=0.000 n=10)
FileCipherStream/fips=false/key=128/block=10240/-24    21.75µ ± 0%   18.49µ ± 0%  -14.97% (p=0.000 n=10)
FileCipherStream/fips=false/key=192/block=16/-24       66.86n ± 1%   60.85n ± 1%   -9.00% (p=0.000 n=10)
FileCipherStream/fips=false/key=192/block=1024/-24     2.381µ ± 0%   2.037µ ± 0%  -14.47% (p=0.000 n=10)
FileCipherStream/fips=false/key=192/block=10240/-24    23.34µ ± 0%   20.02µ ± 0%  -14.24% (p=0.000 n=10)
FileCipherStream/fips=false/key=256/block=16/-24       69.34n ± 1%   63.02n ± 1%   -9.11% (p=0.000 n=10)
FileCipherStream/fips=false/key=256/block=1024/-24     2.541µ ± 0%   2.188µ ± 0%  -13.88% (p=0.000 n=10)
FileCipherStream/fips=false/key=256/block=10240/-24    24.95µ ± 0%   21.51µ ± 0%  -13.80% (p=0.000 n=10)
geomean                                                1.548µ        1.349µ       -12.82%

                                                    │ before.bench │           after-xor.bench            │
                                                    │     B/s      │     B/s       vs base                │
FileCipherStream/fips=false/key=128/block=16/-24      236.1Mi ± 0%   263.7Mi ± 2%  +11.67% (p=0.000 n=10)
FileCipherStream/fips=false/key=128/block=1024/-24    439.7Mi ± 0%   518.3Mi ± 0%  +17.89% (p=0.000 n=10)
FileCipherStream/fips=false/key=128/block=10240/-24   449.0Mi ± 0%   528.1Mi ± 0%  +17.61% (p=0.000 n=10)
FileCipherStream/fips=false/key=192/block=16/-24      228.2Mi ± 1%   250.8Mi ± 1%   +9.89% (p=0.000 n=10)
FileCipherStream/fips=false/key=192/block=1024/-24    410.0Mi ± 0%   479.4Mi ± 0%  +16.93% (p=0.000 n=10)
FileCipherStream/fips=false/key=192/block=10240/-24   418.3Mi ± 0%   487.8Mi ± 0%  +16.61% (p=0.000 n=10)
FileCipherStream/fips=false/key=256/block=16/-24      220.1Mi ± 1%   242.1Mi ± 1%  +10.04% (p=0.000 n=10)
FileCipherStream/fips=false/key=256/block=1024/-24    384.4Mi ± 0%   446.4Mi ± 0%  +16.12% (p=0.000 n=10)
FileCipherStream/fips=false/key=256/block=10240/-24   391.4Mi ± 0%   454.1Mi ± 0%  +16.01% (p=0.000 n=10)
geomean                                               339.8Mi        389.8Mi       +14.71%
bdarnell@gceworker-bdarnell-fips:~/go/src/github.com/cockroachdb/cockroach$ ~/go/bin/benchstat before-fips.bench after-xor-fips.bench
goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                   │ before-fips.bench │        after-xor-fips.bench        │
                                                   │      sec/op       │   sec/op     vs base               │
FileCipherStream/fips=true/key=128/block=16/-24            270.9n ± 0%   259.2n ± 1%  -4.32% (p=0.000 n=10)
FileCipherStream/fips=true/key=128/block=1024/-24          14.86µ ± 0%   14.04µ ± 0%  -5.52% (p=0.000 n=10)
FileCipherStream/fips=true/key=128/block=10240/-24         149.2µ ± 1%   140.8µ ± 1%  -5.60% (p=0.000 n=10)
FileCipherStream/fips=true/key=192/block=16/-24            272.3n ± 1%   260.4n ± 1%  -4.37% (p=0.000 n=10)
FileCipherStream/fips=true/key=192/block=1024/-24          14.98µ ± 1%   14.17µ ± 1%  -5.41% (p=0.000 n=10)
FileCipherStream/fips=true/key=192/block=10240/-24         149.3µ ± 0%   141.9µ ± 1%  -4.92% (p=0.000 n=10)
FileCipherStream/fips=true/key=256/block=16/-24            273.7n ± 1%   261.3n ± 1%  -4.53% (p=0.000 n=10)
FileCipherStream/fips=true/key=256/block=1024/-24          15.02µ ± 1%   14.23µ ± 2%  -5.28% (p=0.000 n=10)
FileCipherStream/fips=true/key=256/block=10240/-24         150.0µ ± 1%   142.4µ ± 1%  -5.09% (p=0.000 n=10)
geomean                                                    8.475µ        8.051µ       -5.01%

                                                   │ before-fips.bench │        after-xor-fips.bench         │
                                                   │        B/s        │     B/s       vs base               │
FileCipherStream/fips=true/key=128/block=16/-24           56.33Mi ± 0%   58.87Mi ± 1%  +4.50% (p=0.000 n=10)
FileCipherStream/fips=true/key=128/block=1024/-24         65.71Mi ± 0%   69.55Mi ± 0%  +5.83% (p=0.000 n=10)
FileCipherStream/fips=true/key=128/block=10240/-24        65.46Mi ± 1%   69.35Mi ± 1%  +5.94% (p=0.000 n=10)
FileCipherStream/fips=true/key=192/block=16/-24           56.04Mi ± 1%   58.60Mi ± 1%  +4.57% (p=0.000 n=10)
FileCipherStream/fips=true/key=192/block=1024/-24         65.20Mi ± 1%   68.93Mi ± 1%  +5.72% (p=0.000 n=10)
FileCipherStream/fips=true/key=192/block=10240/-24        65.42Mi ± 0%   68.80Mi ± 1%  +5.17% (p=0.000 n=10)
FileCipherStream/fips=true/key=256/block=16/-24           55.75Mi ± 1%   58.39Mi ± 1%  +4.74% (p=0.000 n=10)
FileCipherStream/fips=true/key=256/block=1024/-24         65.00Mi ± 1%   68.63Mi ± 2%  +5.58% (p=0.000 n=10)
FileCipherStream/fips=true/key=256/block=10240/-24        65.11Mi ± 1%   68.60Mi ± 1%  +5.36% (p=0.000 n=10)
geomean                                                   62.07Mi        65.33Mi       +5.27%

```

116669: streamingccl: deflake TestStreamingReplanOnLag r=msbutler a=msbutler

The test would timeout under stress as it relies on node 1 falling behind, but occasonially, partitionSpans would not include node 1 in the c2c distSQL plan. This patch prevents this by modifying the CreateScatteredTable to wait until the desired number of nodes have leases.

Epic: none

Release note: none

116869: kvclient: remove unused field from DistSender r=yuzefovich a=andrewbaptist

Remove the unused NodeDescriptor field from DistSender.

Epic: none

Release note: None

116876: logictestccl: skip 3node-tenant config under race r=yuzefovich a=yuzefovich

We just skipped 3node-tenant config under race for non-ccl logic tests and forgot to do so for the ccl ones. This commit fixes that as well as adjusts the reason for the skip.

Fixes: cockroachdb#116854.
Fixes: cockroachdb#116855.

Release note: None

Co-authored-by: Ben Darnell <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
  • Loading branch information
5 people committed Dec 20, 2023
5 parents 0af1fed + c7c888d + d6d6692 + a8be34f + b818079 commit 2c4c877
Show file tree
Hide file tree
Showing 14 changed files with 21 additions and 30 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions pkg/ccl/storageccl/engineccl/ctr_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"crypto/aes"
"crypto/cipher"
"crypto/rand"
"crypto/subtle"
"encoding/binary"
"fmt"

Expand Down Expand Up @@ -192,7 +193,5 @@ func (s *cTRBlockCipherStream) transform(blockIndex uint64, data []byte, scratch
binary.BigEndian.PutUint32(iv[len(iv):len(iv)+4], blockCounter)
iv = iv[0 : len(iv)+4]
s.cBlock.Encrypt(iv, iv)
for i := 0; i < ctrBlockSize; i++ {
data[i] = data[i] ^ iv[i]
}
subtle.XORBytes(data, data, iv)
}
10 changes: 7 additions & 3 deletions pkg/ccl/streamingccl/replicationtestutils/testutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -517,17 +517,21 @@ func CreateScatteredTable(t *testing.T, c *TenantStreamingClusters, numNodes int
c.SrcTenantSQL.Exec(t, "ALTER TABLE d.scattered SPLIT AT (SELECT * FROM generate_series($1::INT, $2::INT, $3::INT))",
rowsPerRange, (numRanges-1)*rowsPerRange, rowsPerRange)
c.SrcTenantSQL.Exec(t, "ALTER TABLE d.scattered SCATTER")
testutils.SucceedsSoon(t, func() error {
timeout := 45 * time.Second
if skip.Duress() {
timeout *= 5
}
testutils.SucceedsWithin(t, func() error {
var leaseHolderCount int
c.SrcTenantSQL.QueryRow(t,
`SELECT count(DISTINCT lease_holder) FROM [SHOW RANGES FROM DATABASE d WITH DETAILS]`).
Scan(&leaseHolderCount)
require.Greater(t, leaseHolderCount, 0)
if leaseHolderCount == 1 {
if leaseHolderCount < numNodes {
return errors.New("leaseholders not scattered yet")
}
return nil
})
}, timeout)
}

var defaultSrcClusterSetting = map[string]string{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,8 @@ func TestStreamingAutoReplan(t *testing.T) {
c.SrcCluster.AddAndStartServer(c.T, replicationtestutils.CreateServerArgs(c.Args))
require.NoError(t, c.SrcCluster.WaitForFullReplication())

replicationtestutils.CreateScatteredTable(t, c, 3)
// Only need at least two nodes as leaseholders for test.
replicationtestutils.CreateScatteredTable(t, c, 2)

// Configure the ingestion job to replan eagerly.
serverutils.SetClusterSetting(t, c.DestCluster, "stream_replication.replan_flow_threshold", 0.1)
Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/generate-logictest/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func runLogicTest(t *testing.T, file string) {
{{- define "runCCLLogicTest" }}
{{- if .CclLogicTest -}}
func runCCLLogicTest(t *testing.T, file string) {
{{ if .IsMultiRegion }}skip.UnderRace(t, "times out and/or OOM's")
{{ if or .IsMultiRegion .Is3NodeTenant }}skip.UnderRace(t, "large engflow executor is overloaded by this config")
{{ end }}skip.UnderDeadlock(t, "times out and/or hangs")
logictest.RunLogicTest(t, logictest.TestServerArgs{}, configIdx, filepath.Join(cclLogicTestDir, file))
}
Expand Down
15 changes: 1 addition & 14 deletions pkg/kv/kvclient/kvcoord/dist_sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"sync"
"sync/atomic"
"time"
"unsafe"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/gossip"
Expand Down Expand Up @@ -511,11 +510,6 @@ type DistSender struct {
log.AmbientContext

st *cluster.Settings
// nodeDescriptor, if set, holds the descriptor of the node the
// DistSender lives on. It should be accessed via getNodeDescriptor(),
// which tries to obtain the value from the Gossip network if the
// descriptor is unknown.
nodeDescriptor unsafe.Pointer
// clock is used to set time for some calls. E.g. read-only ops
// which span ranges and don't require read consistency.
clock *hlc.Clock
Expand Down Expand Up @@ -599,11 +593,7 @@ type DistSenderConfig struct {
// NodeIDGetter, if set, provides non-gossip based implementation for
// obtaining the local KV node ID. The DistSender uses the node ID to
// preferentially route requests to a local replica (if one exists).
NodeIDGetter func() roachpb.NodeID
// nodeDescriptor, if provided, is used to describe which node the
// DistSender lives on, for instance when deciding where to send RPCs.
// Usually it is filled in from the Gossip network on demand.
nodeDescriptor *roachpb.NodeDescriptor
NodeIDGetter func() roachpb.NodeID
RPCRetryOptions *retry.Options
RPCContext *rpc.Context
// NodeDialer is the dialer from the SQL layer to the KV layer.
Expand Down Expand Up @@ -675,9 +665,6 @@ func NewDistSender(cfg DistSenderConfig) *DistSender {
panic("no tracer set in AmbientCtx")
}

if cfg.nodeDescriptor != nil {
atomic.StorePointer(&ds.nodeDescriptor, unsafe.Pointer(cfg.nodeDescriptor))
}
var rdb rangecache.RangeDescriptorDB
if cfg.FirstRangeProvider != nil {
ds.firstRangeProvider = cfg.FirstRangeProvider
Expand Down
1 change: 0 additions & 1 deletion pkg/kv/kvclient/kvcoord/local_test_cluster_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ func NewDistSenderForLocalTestCluster(
NodeDescs: g,
RPCContext: rpcContext,
RPCRetryOptions: &retryOpts,
nodeDescriptor: nodeDesc,
NodeDialer: nodedialer.New(rpcContext, gossip.AddressResolver(g)),
FirstRangeProvider: g,
TestingKnobs: ClientTestingKnobs{
Expand Down

0 comments on commit 2c4c877

Please sign in to comment.