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

distsqlrun: add NoopProcessor benchmark #24477

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Apr 4, 2018

It's nice to be able to see how fast the DistSQL processor interface itself is.

name             time/op
Noop/cols=1-8     3.23ms ± 2%
Noop/cols=2-8     4.56ms ± 2%
Noop/cols=4-8     6.53ms ± 5%
Noop/cols=16-8    23.4ms ± 2%
Noop/cols=256-8    407ms ± 6%

name             speed
Noop/cols=1-8    162MB/s ± 2%
Noop/cols=2-8    230MB/s ± 2%
Noop/cols=4-8    321MB/s ± 5%
Noop/cols=16-8   359MB/s ± 2%
Noop/cols=256-8  331MB/s ± 5%

@jordanlewis jordanlewis requested review from solongordon, rjnn, asubiotto and a team April 4, 2018 16:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@solongordon
Copy link
Contributor

:lgtm:


Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@asubiotto
Copy link
Contributor

Reviewed 1 of 2 files at r1.
Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, some commit checks pending.


pkg/sql/distsqlrun/noop_test.go, line 31 at r2 (raw file):

	ctx := context.Background()
	st := cluster.MakeTestingClusterSettings()

I think you can just embed this in &FlowCtx{...


pkg/sql/distsqlrun/noop_test.go, line 52 at r2 (raw file):

			b.ResetTimer()
			for i := 0; i < b.N; i++ {
				d, err := newNoopProcessor(flowCtx, input, post, &RowDisposer{})

Probably good to be consistent here and extract the RowDisposer to where post is allocated.


Comments from Reviewable

@asubiotto
Copy link
Contributor

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


pkg/sql/distsqlrun/noop_test.go, line 31 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think you can just embed this in &FlowCtx{...

Nevermind, didn't see the use in evalCtx := ...


Comments from Reviewable

@petermattis
Copy link
Collaborator

Can you include the benchmark results (using benchstat)? Inquiring minds want to know and are too lazy to run the benchmark themselves.


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


Comments from Reviewable

@jordanlewis
Copy link
Member Author

Here are the results (updated the PR description as well):

name             time/op
Noop/cols=1-8     3.23ms ± 2%
Noop/cols=2-8     4.56ms ± 2%
Noop/cols=4-8     6.53ms ± 5%
Noop/cols=16-8    23.4ms ± 2%
Noop/cols=256-8    407ms ± 6%

name             speed
Noop/cols=1-8    162MB/s ± 2%
Noop/cols=2-8    230MB/s ± 2%
Noop/cols=4-8    321MB/s ± 5%
Noop/cols=16-8   359MB/s ± 2%
Noop/cols=256-8  331MB/s ± 5%

```
name             time/op
Noop/cols=1-8     3.23ms ± 2%
Noop/cols=2-8     4.56ms ± 2%
Noop/cols=4-8     6.53ms ± 5%
Noop/cols=16-8    23.4ms ± 2%
Noop/cols=256-8    407ms ± 6%

name             speed
Noop/cols=1-8    162MB/s ± 2%
Noop/cols=2-8    230MB/s ± 2%
Noop/cols=4-8    321MB/s ± 5%
Noop/cols=16-8   359MB/s ± 2%
Noop/cols=256-8  331MB/s ± 5%
```

Release note: None
@jordanlewis
Copy link
Member Author

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


pkg/sql/distsqlrun/noop_test.go, line 52 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Probably good to be consistent here and extract the RowDisposer to where post is allocated.

Done.


Comments from Reviewable

@asubiotto
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@jordanlewis
Copy link
Member Author

bors r+

craig bot added a commit that referenced this pull request Apr 4, 2018
24477: distsqlrun: add NoopProcessor benchmark r=jordanlewis a=jordanlewis

It's nice to be able to see how fast the DistSQL processor interface itself is.

```
name             time/op
Noop/cols=1-8     3.23ms ± 2%
Noop/cols=2-8     4.56ms ± 2%
Noop/cols=4-8     6.53ms ± 5%
Noop/cols=16-8    23.4ms ± 2%
Noop/cols=256-8    407ms ± 6%

name             speed
Noop/cols=1-8    162MB/s ± 2%
Noop/cols=2-8    230MB/s ± 2%
Noop/cols=4-8    321MB/s ± 5%
Noop/cols=16-8   359MB/s ± 2%
Noop/cols=256-8  331MB/s ± 5%
```
@craig
Copy link
Contributor

craig bot commented Apr 4, 2018

Build succeeded

@craig craig bot merged commit 5a2cc78 into cockroachdb:master Apr 4, 2018
@jordanlewis jordanlewis deleted the benchmark-noop branch April 5, 2018 16:21
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