Merge #55062 #55214 #55277 #55280

55062: kvserver: remove unused parameter r=TheSamHuang a=tbg Release note: None 55214: sql: fix performance regression due to increased hash allocations r=nvanbenschoten,knz a=arulajmani We recently increased allocations in the `appStats` structure when adding support for transaction level statistics in #52704. This was because the interactions with the `fnv` library were expensive in terms of allocations. This patch aims to claw back the regression by: - Using our own implementation of the FNV algorithm instead of the library, which is significantly lighter weight (microbenchmarks below). - Re-organizes the code to only construct the statement IDs (deemed the expensive operation) if required. When comparing the difference between the commit that introduced the regression and the changes proposed by this diff, I got the following improvements on the KV workload: ``` name old ops/s new ops/s delta kv95-throughput 34.5k ± 6% 35.7k ± 4% +3.42% (p=0.023 n=10+10) ``` Microbenchmarks for the new hashing algorithm (written/run by @knz): ``` name old time/op new time/op delta ConstructStatementID-32 405ns ±17% 39ns ±12% -90.34% (p=0.008 n=5+5) name old alloc/op new alloc/op delta ConstructStatementID-32 120B ± 0% 16B ± 0% -86.67% (p=0.008 n=5+5) name old allocs/op new allocs/op delta ConstructStatementID-32 6.00 ± 0% 1.00 ± 0% -83.33% (p=0.008 n=5+5) ``` Closes #54515 Release note: None 55277: server: create engines in NewServer r=irfansharif a=tbg We were jumping through a number of hoops to create the engines only in `(*Server).Start` since that seems to be the "idiomatic" place to start moving parts. However, it creates a lot of complexity since various callbacks have to be registered with access to engines. Move engine creation to `NewServer`. This unblocks #54936. Release note: None 55280: roachtest: recognize GEOS dlls on more platforms r=otan a=knz This makes roachtest work on the BSDs again. Release note: None Co-authored-by: Tobias Grieger <[email protected]> Co-authored-by: arulajmani <[email protected]> Co-authored-by: David Hartunian <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Oct 7, 2020 · 04bfabb · 04bfabb
5 parents f5fed87 + 7ca1c56 + 9757d77 + 7abe7d7 + 18be6a7
commit 04bfabb
Show file tree

Hide file tree

Showing 24 changed files with 434 additions and 381 deletions.
diff --git a/docs/generated/http/full.md b/docs/generated/http/full.md
@@ -1813,7 +1813,7 @@ information about the resources on a node used by that table.
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | key | [StatementsResponse.ExtendedStatementStatisticsKey](#cockroach.server.serverpb.StatementsResponse-cockroach.server.serverpb.StatementsResponse.ExtendedStatementStatisticsKey) |  |  |
-| id | [string](#cockroach.server.serverpb.StatementsResponse-string) |  |  |
+| id | [uint64](#cockroach.server.serverpb.StatementsResponse-uint64) |  |  |
 | stats | [cockroach.sql.StatementStatistics](#cockroach.server.serverpb.StatementsResponse-cockroach.sql.StatementStatistics) |  |  |
 
 

diff --git a/pkg/cmd/roachtest/cluster.go b/pkg/cmd/roachtest/cluster.go
@@ -136,6 +136,9 @@ func findLibrary(libraryName string) (string, error) {
 	if local {
 		switch runtime.GOOS {
 		case "linux":
+		case "freebsd":
+		case "openbsd":
+		case "dragonfly":
 		case "windows":
 			suffix = ".dll"
 		case "darwin":

diff --git a/pkg/kv/kvserver/raft_transport.go b/pkg/kv/kvserver/raft_transport.go
@@ -20,7 +20,6 @@ import (
 	"time"
 	"unsafe"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
@@ -653,23 +652,20 @@ func (t *RaftTransport) startProcessNewQueue(
 // for closing the OutgoingSnapshot.
 func (t *RaftTransport) SendSnapshot(
 	ctx context.Context,
-	raftCfg *base.RaftConfig,
 	storePool *StorePool,
 	header SnapshotRequest_Header,
 	snap *OutgoingSnapshot,
 	newBatch func() storage.Batch,
 	sent func(),
 ) error {
-	var stream MultiRaft_RaftSnapshotClient
 	nodeID := header.RaftMessageRequest.ToReplica.NodeID
 
 	conn, err := t.dialer.Dial(ctx, nodeID, rpc.DefaultClass)
 	if err != nil {
 		return err
 	}
-
 	client := NewMultiRaftClient(conn)
-	stream, err = client.RaftSnapshot(ctx)
+	stream, err := client.RaftSnapshot(ctx)
 	if err != nil {
 		return err
 	}
@@ -679,5 +675,7 @@ func (t *RaftTransport) SendSnapshot(
 			log.Warningf(ctx, "failed to close snapshot stream: %+v", err)
 		}
 	}()
-	return sendSnapshot(ctx, raftCfg, t.st, stream, storePool, header, snap, newBatch, sent)
+	return sendSnapshot(
+		ctx, t.st, stream, storePool, header, snap, newBatch, sent,
+	)
 }
diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go
@@ -1928,7 +1928,6 @@ func (r *Replica) sendSnapshot(
 	}
 	if err := r.store.cfg.Transport.SendSnapshot(
 		ctx,
-		&r.store.cfg.RaftConfig,
 		r.store.allocator.storePool,
 		req,
 		snap,

diff --git a/pkg/kv/kvserver/replica_sideload_test.go b/pkg/kv/kvserver/replica_sideload_test.go
@@ -825,7 +825,6 @@ func TestRaftSSTableSideloadingSnapshot(t *testing.T) {
 		mockSender := &mockSender{}
 		if err := sendSnapshot(
 			ctx,
-			&tc.store.cfg.RaftConfig,
 			tc.store.cfg.Settings,
 			mockSender,
 			&fakeStorePool{},
@@ -947,7 +946,6 @@ func TestRaftSSTableSideloadingSnapshot(t *testing.T) {
 		mockSender := &mockSender{}
 		err = sendSnapshot(
 			ctx,
-			&tc.store.cfg.RaftConfig,
 			tc.store.cfg.Settings,
 			mockSender,
 			&fakeStorePool{},

diff --git a/pkg/kv/kvserver/store_snapshot.go b/pkg/kv/kvserver/store_snapshot.go
@@ -16,7 +16,6 @@ import (
 	"io"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -91,8 +90,7 @@ func assertStrategy(
 // kvBatchSnapshotStrategy is an implementation of snapshotStrategy that streams
 // batches of KV pairs in the BatchRepr format.
 type kvBatchSnapshotStrategy struct {
-	raftCfg *base.RaftConfig
-	status  string
+	status string
 
 	// The size of the batches of PUT operations to send to the receiver of the
 	// snapshot. Only used on the sender side.
@@ -781,7 +779,6 @@ func (s *Store) receiveSnapshot(
 		}
 
 		ss = &kvBatchSnapshotStrategy{
-			raftCfg:      &s.cfg.RaftConfig,
 			scratch:      s.sstSnapshotStorage.NewScratchSpace(header.State.Desc.RangeID, snapUUID),
 			sstChunkSize: snapshotSSTWriteSyncRate.Get(&s.cfg.Settings.SV),
 		}
@@ -901,7 +898,6 @@ func (e *errMustRetrySnapshotDueToTruncation) Error() string {
 // sendSnapshot sends an outgoing snapshot via a pre-opened GRPC stream.
 func sendSnapshot(
 	ctx context.Context,
-	raftCfg *base.RaftConfig,
 	st *cluster.Settings,
 	stream outgoingSnapshotStream,
 	storePool SnapshotStorePool,
@@ -976,7 +972,6 @@ func sendSnapshot(
 	switch header.Strategy {
 	case SnapshotRequest_KV_BATCH:
 		ss = &kvBatchSnapshotStrategy{
-			raftCfg:   raftCfg,
 			batchSize: batchSize,
 			limiter:   limiter,
 			newBatch:  newBatch,

diff --git a/pkg/kv/kvserver/store_test.go b/pkg/kv/kvserver/store_test.go
@@ -3054,8 +3054,6 @@ func TestSendSnapshotThrottling(t *testing.T) {
 	defer e.Close()
 
 	ctx := context.Background()
-	var cfg base.RaftConfig
-	cfg.SetDefaults()
 	st := cluster.MakeTestingClusterSettings()
 
 	header := SnapshotRequest_Header{
@@ -3071,7 +3069,7 @@ func TestSendSnapshotThrottling(t *testing.T) {
 		sp := &fakeStorePool{}
 		expectedErr := errors.New("")
 		c := fakeSnapshotStream{nil, expectedErr}
-		err := sendSnapshot(ctx, &cfg, st, c, sp, header, nil, newBatch, nil)
+		err := sendSnapshot(ctx, st, c, sp, header, nil, newBatch, nil)
 		if sp.failedThrottles != 1 {
 			t.Fatalf("expected 1 failed throttle, but found %d", sp.failedThrottles)
 		}
@@ -3087,7 +3085,7 @@ func TestSendSnapshotThrottling(t *testing.T) {
 			Status: SnapshotResponse_DECLINED,
 		}
 		c := fakeSnapshotStream{resp, nil}
-		err := sendSnapshot(ctx, &cfg, st, c, sp, header, nil, newBatch, nil)
+		err := sendSnapshot(ctx, st, c, sp, header, nil, newBatch, nil)
 		if sp.declinedThrottles != 1 {
 			t.Fatalf("expected 1 declined throttle, but found %d", sp.declinedThrottles)
 		}
@@ -3104,7 +3102,7 @@ func TestSendSnapshotThrottling(t *testing.T) {
 			Status: SnapshotResponse_DECLINED,
 		}
 		c := fakeSnapshotStream{resp, nil}
-		err := sendSnapshot(ctx, &cfg, st, c, sp, header, nil, newBatch, nil)
+		err := sendSnapshot(ctx, st, c, sp, header, nil, newBatch, nil)
 		if sp.failedThrottles != 1 {
 			t.Fatalf("expected 1 failed throttle, but found %d", sp.failedThrottles)
 		}
@@ -3120,7 +3118,7 @@ func TestSendSnapshotThrottling(t *testing.T) {
 			Status: SnapshotResponse_ERROR,
 		}
 		c := fakeSnapshotStream{resp, nil}
-		err := sendSnapshot(ctx, &cfg, st, c, sp, header, nil, newBatch, nil)
+		err := sendSnapshot(ctx, st, c, sp, header, nil, newBatch, nil)
 		if sp.failedThrottles != 1 {
 			t.Fatalf("expected 1 failed throttle, but found %d", sp.failedThrottles)
 		}

diff --git a/pkg/roachpb/app_stats.go b/pkg/roachpb/app_stats.go
@@ -11,28 +11,34 @@
 package roachpb
 
 import (
-	"fmt"
-	"hash/fnv"
 	"math"
+
+	"github.com/cockroachdb/cockroach/pkg/util"
 )
 
 // StmtID is the type of a Statement ID.
-type StmtID string
+type StmtID uint64
 
 // ConstructStatementID constructs an ID by hashing an anonymized query, it's
 // failure status, and if it was part of an implicit txn. At the time of writing,
 // these are the axis' we use to bucket queries for stats collection
 // (see stmtKey).
 func ConstructStatementID(anonymizedStmt string, failed bool, implicitTxn bool) StmtID {
-	h := fnv.New128()
-	h.Write([]byte(anonymizedStmt))
+	fnv := util.MakeFNV64()
+	for _, c := range anonymizedStmt {
+		fnv.Add(uint64(c))
+	}
 	if failed {
-		h.Write([]byte("failed"))
+		fnv.Add('F')
+	} else {
+		fnv.Add('S')
 	}
 	if implicitTxn {
-		h.Write([]byte("implicit_txn"))
+		fnv.Add('I')
+	} else {
+		fnv.Add('E')
 	}
-	return StmtID(fmt.Sprintf("%x", h.Sum(nil)))
+	return StmtID(fnv.Sum())
 }
 
 // GetVariance retrieves the variance of the values.
-Original file line number
+Diff line change
@@ Expand Up / @@ -1928,7 +1928,6 @@ func (r *Replica) sendSnapshot( @@
     	}
     	if err := r.store.cfg.Transport.SendSnapshot(
     		ctx,
-    		&r.store.cfg.RaftConfig,
     		r.store.allocator.storePool,
     		req,
     		snap,
@@ Expand Down @@