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: run last processor from main goroutine #27899

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

asubiotto
Copy link
Contributor

Closes #27734

This change improves distsql throughput for short-running queries by
removing some synchronization overhead. On a kv --read-percent=100
workload, this change resulted in a 25% throughput improvement (however note that these numbers vary a fair amount).

Release note (performance improvement): Improve fixed cost of running
distributed sql queries.

_elapsed___errors_____ops(total)___ops/sec(cum)__avg(ms)__p50(ms)__p95(ms)__p99(ms)_pMax(ms)__result
   60.0s        0         690447        11507.2      1.4      1.1      3.3      5.8    130.0
_elapsed___errors_____ops(total)___ops/sec(cum)__avg(ms)__p50(ms)__p95(ms)__p99(ms)_pMax(ms)__result
   60.0s        0         865636        14427.7      1.1      0.9      2.6      4.2     39.8

@asubiotto asubiotto requested review from a team July 24, 2018 20:14
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/distsqlrun/flow.go, line 469 at r1 (raw file):

// setup error is pushed to the syncFlowConsumer. In this case, a subsequent
// call to f.Wait() will not block.
func (f *Flow) Start(ctx context.Context, block bool, doneFn func()) error {

Rather than add a boolean parameter, I'd recommend adding a new method called StartSync and renaming this one to StartAsync, or similar. That will make it much more clear in each of the call sites what's actually going on.

Copy link
Contributor Author

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/distsqlrun/flow.go, line 469 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Rather than add a boolean parameter, I'd recommend adding a new method called StartSync and renaming this one to StartAsync, or similar. That will make it much more clear in each of the call sites what's actually going on.

Good call. Done.

@asubiotto asubiotto force-pushed the gperf branch 3 times, most recently from 9605bf2 to 8999200 Compare July 25, 2018 14:48
Closes #27734

This change improves distsql throughput for short-running queries by
removing some synchronization overhead. On a kv --read-percent=100
workload, this change resulted in a 25% throughput improvement.

Release note (performance improvement): Improve fixed cost of running
distributed sql queries.
@asubiotto
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Jul 25, 2018
27899: distsql: run last processor from main goroutine r=asubiotto a=asubiotto

Closes #27734

This change improves distsql throughput for short-running queries by
removing some synchronization overhead. On a kv --read-percent=100
workload, this change resulted in a 25% throughput improvement (however note that these numbers vary a fair amount).

Release note (performance improvement): Improve fixed cost of running
distributed sql queries.

```
_elapsed___errors_____ops(total)___ops/sec(cum)__avg(ms)__p50(ms)__p95(ms)__p99(ms)_pMax(ms)__result
   60.0s        0         690447        11507.2      1.4      1.1      3.3      5.8    130.0
```

```
_elapsed___errors_____ops(total)___ops/sec(cum)__avg(ms)__p50(ms)__p95(ms)__p99(ms)_pMax(ms)__result
   60.0s        0         865636        14427.7      1.1      0.9      2.6      4.2     39.8
```

Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jul 25, 2018

Build succeeded

@craig craig bot merged commit df62b9b into cockroachdb:master Jul 25, 2018
@asubiotto asubiotto deleted the gperf branch July 31, 2018 22:27
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.

distsqlrun: distsqlreceiver should be run from main goroutine
3 participants