New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

distsql: pool physical plans #30884

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jordanlewis
Member

jordanlewis commented Oct 2, 2018

First 2 commits are part of #30607

name                           old time/op    new time/op    delta
FlowSetup/distribute=false-24    16.3µs ± 2%    15.9µs ± 2%  -1.96%
(p=0.001 n=8+9)

name                           old alloc/op   new alloc/op   delta
FlowSetup/distribute=false-24    25.6kB ± 0%    25.1kB ± 0%  -2.29%
(p=0.000 n=10+10)

name                           old allocs/op  new allocs/op  delta
FlowSetup/distribute=false-24       246 ± 0%       243 ± 0%  -1.22%
(p=0.000 n=10+10)

@jordanlewis jordanlewis requested review from cockroachdb/distsql-prs as code owners Oct 2, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 2, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 2, 2018

This change is Reviewable

jordanlewis added some commits Sep 21, 2018

sql: pass PhysicalPlan by pointer
It always gets allocated eventually anyway. An upcoming commit will
introduce pooling of these objects as well.

Release note: None
sql: pool PhysicalPlan objects
```
name                           old time/op    new time/op    delta
FlowSetup/distribute=false-24    16.3µs ± 2%    15.9µs ± 2%  -1.96%
(p=0.001 n=8+9)

name                           old alloc/op   new alloc/op   delta
FlowSetup/distribute=false-24    25.6kB ± 0%    25.1kB ± 0%  -2.29%
(p=0.000 n=10+10)

name                           old allocs/op  new allocs/op  delta
FlowSetup/distribute=false-24       246 ± 0%       243 ± 0%  -1.22%
(p=0.000 n=10+10)
```

Release note: None
@RaduBerinde

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/distsql_physical_planner.go, line 1098 at r2 (raw file):

	nPartitions := len(spanPartitions)
	if cap(p.ResultRouters) >= nPartitions {
		p.ResultRouters = p.ResultRouters[:nPartitions]

We may be relying on these slices starting out as zero. In this case we are resurrecting the last state. (btw I'd go back and check your other similar changes with this in mind).

I believe that go 1.11 recognizes statements like append(s, make([]T, n)...) and avoids the allocation. So these could all become x = append(x[:0], make([]T, n)...), assuming you're not looking to backport.

@jordanlewis

This comment has been minimized.

Show comment
Hide comment
@jordanlewis

jordanlewis Oct 4, 2018

Member

Yeah I thought I was fastidious about checking for assumptions like that - I must have missed some. Also we de-upgraded from Go 1.11, or else I'd use your syntax.

Member

jordanlewis commented Oct 4, 2018

Yeah I thought I was fastidious about checking for assumptions like that - I must have missed some. Also we de-upgraded from Go 1.11, or else I'd use your syntax.

@asubiotto

Reviewed 7 of 7 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/distsql_physical_planner.go, line 1153 at r2 (raw file):

		nTypes = len(n.desc.Columns)
	}
	types := p.ResultTypes[:0]

nit: This reslicing is unnecessary I think.


pkg/sql/distsql_physical_planner.go, line 2031 at r2 (raw file):

}

var physicalPlanPool = sync.Pool{

I would stick these next to PhysicalPlan.Release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment