From 5b7f18888794b636075034bc1d9bd23e42f117bb Mon Sep 17 00:00:00 2001
From: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Date: Mon, 7 Oct 2019 00:08:21 -0400
Subject: [PATCH] sql: don't allocate phaseTimes on each call to
 sqlStatsCollector.reset

This allocation was responsible for **0.28%** of a CPU profile when running
Sysbench's oltp_point_select workload.

This was broken in ee48ea7.

Release justification: Reverts perf regression due to newly introduced memory
allocations.

Release note: None
---
 pkg/sql/conn_executor.go              |  4 ++--
 pkg/sql/conn_executor_exec.go         |  6 +++---
 pkg/sql/conn_executor_prepare.go      |  2 +-
 pkg/sql/exec_util.go                  | 10 +++++-----
 pkg/sql/executor_statement_metrics.go |  2 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go
index 5024eb654ca1..d14a8d2a840c 100644
--- a/pkg/sql/conn_executor.go
+++ b/pkg/sql/conn_executor.go
@@ -1635,7 +1635,7 @@ func (ex *connExecutor) execCopyIn(
 			// going through the state machine.
 			ex.state.sqlTimestamp = txnTS
 			ex.statsCollector = ex.newStatsCollector()
-			ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, ex.phaseTimes)
+			ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
 			ex.initPlanner(ctx, p)
 			ex.resetPlanner(ctx, p, txn, stmtTS, 0 /* numAnnotations */)
 		},
@@ -2079,7 +2079,7 @@ func (ex *connExecutor) recordError(ctx context.Context, err error) {
 // newStatsCollector returns a sqlStatsCollector that will record stats in the
 // session's stats containers.
 func (ex *connExecutor) newStatsCollector() *sqlStatsCollector {
-	return newSQLStatsCollector(&ex.server.sqlStats, ex.appStats, ex.phaseTimes)
+	return newSQLStatsCollector(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
 }
 
 // cancelQuery is part of the registrySession interface.
diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go
index 9435af940e5f..545bf23a1fd9 100644
--- a/pkg/sql/conn_executor_exec.go
+++ b/pkg/sql/conn_executor_exec.go
@@ -362,7 +362,7 @@ func (ex *connExecutor) execStmtInOpenState(
 
 	p := &ex.planner
 	stmtTS := ex.server.cfg.Clock.PhysicalTime()
-	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, ex.phaseTimes)
+	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
 	ex.resetPlanner(ctx, p, ex.state.mu.txn, stmtTS, stmt.NumAnnotations)
 
 	if os.ImplicitTxn.Get() {
@@ -863,7 +863,7 @@ func (ex *connExecutor) beginTransactionTimestampsAndReadMode(
 		rwMode = ex.readWriteModeWithSessionDefault(s.Modes.ReadWriteMode)
 		return rwMode, now.GoTime(), nil, nil
 	}
-	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, ex.phaseTimes)
+	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
 	p := &ex.planner
 	ex.resetPlanner(ctx, p, nil /* txn */, now.GoTime(), 0 /* numAnnotations */)
 	ts, err := p.EvalAsOfTimestamp(s.Modes.AsOf)
@@ -1300,7 +1300,7 @@ func (ex *connExecutor) recordTransactionStart() func(txnEvent) {
 }
 
 func (ex *connExecutor) recordTransaction(ev txnEvent, implicit bool) {
-	phaseTimes := ex.statsCollector.phaseTimes
+	phaseTimes := &ex.statsCollector.phaseTimes
 	phaseTimes[transactionEnd] = timeutil.Now()
 	txnStart := phaseTimes[transactionStart]
 	txnEnd := phaseTimes[transactionEnd]
diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go
index 6b2daa8b09cb..3dbd72039430 100644
--- a/pkg/sql/conn_executor_prepare.go
+++ b/pkg/sql/conn_executor_prepare.go
@@ -152,7 +152,7 @@ func (ex *connExecutor) prepare(
 	// anything other than getting a timestamp.
 	txn := client.NewTxn(ctx, ex.server.cfg.DB, ex.server.cfg.NodeID.Get(), client.RootTxn)
 
-	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, ex.phaseTimes)
+	ex.statsCollector.reset(&ex.server.sqlStats, ex.appStats, &ex.phaseTimes)
 	p := &ex.planner
 	ex.resetPlanner(ctx, p, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */, stmt.NumAnnotations)
 	p.stmt = &stmt
diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go
index 4841e5581550..d3d7e32ac83d 100644
--- a/pkg/sql/exec_util.go
+++ b/pkg/sql/exec_util.go
@@ -1875,18 +1875,18 @@ type sqlStatsCollector struct {
 	// into sqlStats set as the session's current app.
 	appStats *appStats
 	// phaseTimes tracks session-level phase times.
-	phaseTimes *phaseTimes
+	phaseTimes phaseTimes
 }
 
 // newSQLStatsCollector creates an instance of sqlStatsCollector. Note that
 // phaseTimes is an array, not a slice, so this performs a copy-by-value.
 func newSQLStatsCollector(
-	sqlStats *sqlStats, appStats *appStats, phaseTimes phaseTimes,
+	sqlStats *sqlStats, appStats *appStats, phaseTimes *phaseTimes,
 ) *sqlStatsCollector {
 	return &sqlStatsCollector{
 		sqlStats:   sqlStats,
 		appStats:   appStats,
-		phaseTimes: &phaseTimes,
+		phaseTimes: *phaseTimes,
 	}
 }
 
@@ -1914,10 +1914,10 @@ func (s *sqlStatsCollector) recordTransaction(txnTimeSec float64, ev txnEvent, i
 	s.appStats.recordTransaction(txnTimeSec, ev, implicit)
 }
 
-func (s *sqlStatsCollector) reset(sqlStats *sqlStats, appStats *appStats, phaseTimes phaseTimes) {
+func (s *sqlStatsCollector) reset(sqlStats *sqlStats, appStats *appStats, phaseTimes *phaseTimes) {
 	*s = sqlStatsCollector{
 		sqlStats:   sqlStats,
 		appStats:   appStats,
-		phaseTimes: &phaseTimes,
+		phaseTimes: *phaseTimes,
 	}
 }
diff --git a/pkg/sql/executor_statement_metrics.go b/pkg/sql/executor_statement_metrics.go
index cce20fe71254..bdbe8df78ca5 100644
--- a/pkg/sql/executor_statement_metrics.go
+++ b/pkg/sql/executor_statement_metrics.go
@@ -128,7 +128,7 @@ func (ex *connExecutor) recordStatementSummary(
 	bytesRead int64,
 	rowsRead int64,
 ) {
-	phaseTimes := ex.statsCollector.phaseTimes
+	phaseTimes := &ex.statsCollector.phaseTimes
 
 	// Compute the run latency. This is always recorded in the
 	// server metrics.