Skip to content

Commit

Permalink
coldata,sql: remove some todos
Browse files Browse the repository at this point in the history
This commit removes several TODOs that I have prototyped addressing and
decided to abandon the prototypes, namely:
- checking whether `coldata.BatchSize()` atomic has influence on
performance (the benchmarks and TPCH queries showed that the impact is
negligible if any)
- tuning default batch size (I did that a while ago, and the best
batch size according tpchvec/bench was 1280, barely better than current
1024 which is a lot nicer number)
- pooling allocations of `execFactory` objects (this showed some
improvement on one workload and a regression on another).

Release justification: non-production code changes.

Release note: None
  • Loading branch information
yuzefovich committed Aug 31, 2020
1 parent d102a66 commit 34c79fc
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 7 deletions.
9 changes: 5 additions & 4 deletions pkg/col/coldata/batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,16 @@ type Batch interface {

var _ Batch = &MemBatch{}

// TODO(jordan): tune.
// defaultBatchSize is the size of batches that is used in the non-test setting.
// Initially, 1024 was picked based on MonetDB/X100 paper and was later
// confirmed to be very good using tpchvec/bench benchmark on TPC-H queries
// (the best number according to that benchmark was 1280, but it was negligibly
// better, so we decided to keep 1024 as it is a power of 2).
const defaultBatchSize = 1024

var batchSize int64 = defaultBatchSize

// BatchSize is the maximum number of tuples that fit in a column batch.
// TODO(yuzefovich): we are treating this method almost as if it were a
// constant while it performs an atomic operation. Think through whether it has
// a noticeable performance hit.
func BatchSize() int {
return int(atomic.LoadInt64(&batchSize))
}
Expand Down
3 changes: 0 additions & 3 deletions pkg/sql/plan_opt.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,6 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error {
}

// Build the plan tree.
// TODO(yuzefovich): we're creating a new exec.Factory for every query, but
// we probably could pool those allocations using sync.Pool. Investigate
// this.
if mode := p.SessionData().ExperimentalDistSQLPlanningMode; mode != sessiondata.ExperimentalDistSQLPlanningOff {
planningMode := distSQLDefaultPlanning
// If this transaction has modified or created any types, it is not safe to
Expand Down

0 comments on commit 34c79fc

Please sign in to comment.