Skip to content
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: avoid unnecessary flushes in outbox #22471

Merged
merged 1 commit into from Feb 7, 2018

Conversation

asubiotto
Copy link
Contributor

Previously, the outbox would have a flush ticker that would fire whether
or not the outbox was empty. This change makes it so that we only ever
fire if we have rows to flush.

Addresses #22385

Release note (performance improvement): performance improvement for
distributed sql queries.

SELECT tpch.lineitem ORDER BY l_extendedprice LIMIT 1000:16% improvement on a local 3-node cluster, 22% improvement on a 6-node roachprod cluster.

This should be an across-the-board improvement noticed more with longer-running queries.

@asubiotto asubiotto requested review from petermattis and a team February 7, 2018 19:14
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde
Copy link
Member

pkg/sql/distsqlrun/outbox.go, line 246 at r1 (raw file):

				// this is our first row, restart the flushTimer.
				if m.numRows == 1 {
					flushTimer.Reset(outboxFlushPeriod)

See the doc, I think Stop needs to be called.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/sql/distsqlrun/outbox.go, line 201 at r1 (raw file):

	// TODO(asubiotto): Not sure if this is necessary. I want to avoid a useless
	// flush.
	flushTimer := time.NewTimer(0)

time.Timer is very hard to use correctly. You should use timeutil.NewTimer instead which is a small wrapper. See the comment above timeutil.Timer for usage guidance. The upshot is that you create the timer as

var flushTimer timeutil.Timer
defer flushTimer.Stop()

pkg/sql/distsqlrun/outbox.go, line 246 at r1 (raw file):

Previously, RaduBerinde wrote…

See the doc, I think Stop needs to be called.

See my comment above. time.Timer is very hard to use correctly, thus we have timeutil.Timer.


Comments from Reviewable

@RaduBerinde
Copy link
Member

pkg/sql/distsqlrun/outbox.go, line 201 at r1 (raw file):

	// TODO(asubiotto): Not sure if this is necessary. I want to avoid a useless
	// flush.
	flushTimer := time.NewTimer(0)

not a big deal, but this will probably make us go to sleep and require some other goroutine to wake us up immediately. Maybe just start with a large timeout


Comments from Reviewable

@petermattis
Copy link
Collaborator

Very nice that this achieves the performance benefit you were previously seeing with a longer flush interval.


Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.


Comments from Reviewable

Previously, the outbox would have a flush ticker that would fire whether
or not the outbox was empty. This change makes it so that we only ever
fire if we have rows to flush.

Addresses cockroachdb#22385

Release note (performance improvement): performance improvement for
distributed sql queries.
@asubiotto
Copy link
Contributor Author

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.


pkg/sql/distsqlrun/outbox.go, line 201 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

time.Timer is very hard to use correctly. You should use timeutil.NewTimer instead which is a small wrapper. See the comment above timeutil.Timer for usage guidance. The upshot is that you create the timer as

var flushTimer timeutil.Timer
defer flushTimer.Stop()

Done.


pkg/sql/distsqlrun/outbox.go, line 201 at r1 (raw file):

Previously, RaduBerinde wrote…

not a big deal, but this will probably make us go to sleep and require some other goroutine to wake us up immediately. Maybe just start with a large timeout

Solved by the use of timeutil.Timer


Comments from Reviewable

@jordanlewis
Copy link
Member

:lgtm: the ratio of lines changed to performance improvement on this one is off the charts - awesome!


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@RaduBerinde
Copy link
Member

:lgtm:


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


Comments from Reviewable

@petermattis
Copy link
Collaborator

:lgtm:

You mentioned earlier that you saw a 25% improvement from a larger outboxBufRows and longer ouboxFlushPeriod. I'm wondering if there is a bit of further experimentation here (i.e. bumping outboxBufRows), or if that earlier 25% was really 22%.


Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@asubiotto
Copy link
Contributor Author

Maybe, I'll definitely do some more experimenting.

@asubiotto asubiotto merged commit 4e1bf7f into cockroachdb:master Feb 7, 2018
@asubiotto asubiotto deleted the outbox-perf branch February 7, 2018 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants