Skip to content

Commit

Permalink
sql: don't allocate phaseTimes on each call to sqlStatsCollector.reset
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nvanbenschoten committed Oct 8, 2019
1 parent 27ea46b commit 5b7f188
Show file tree
Hide file tree
Showing 5 changed files with 12 additions and 12 deletions.
4 changes: 2 additions & 2 deletions pkg/sql/conn_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 */)
},
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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]
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/conn_executor_prepare.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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,
}
}
2 changes: 1 addition & 1 deletion pkg/sql/executor_statement_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 5b7f188

Please sign in to comment.