*: standardize all sub-benchmarks on key=value format

[tamird]

No description provided.

[cockroach-teamcity]

This change is 

[tbg]

LGTM. Skimmed this only.

[tamird]

I'd like to get everyone's "ack" on this, at least, since new benchmarks should be written in this style as well.

[petermattis]

Might want to drop a page on the github wiki explaining (briefly) why this is useful/important.

---

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

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

	for _, inputSize := range []int{0, 1 << 2, 1 << 4, 1 << 8, 1 << 12, 1 << 16} {
		b.Run(fmt.Sprintf("InputSize=%d", inputSize), func(b *testing.B) {

Moving the b.Run call might affect the timing. I imagine the timer only starts when b.Run is called. I think I'd revert this part of the change unless you had a strong reason for doing so.

PS We're not consistent in capitalization for these "keys". Does that matter?

---

Comments from Reviewable

[tamird]

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

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Moving the b.Run call might affect the timing. I imagine the timer only starts when b.Run is called. I think I'd revert this part of the change unless you had a strong reason for doing so.

PS We're not consistent in capitalization for these "keys". Does that matter?

Ah, I moved the call to b.Run so that failures here would only tank the sub-bench rather than the top-level one.

Looking at this more, are we intentionally measuring the time taken by rowSource.Reset? @arjunravinarayan @asubiotto

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Ah, I moved the call to b.Run so that failures here would only tank the sub-bench rather than the top-level one.

Looking at this more, are we intentionally measuring the time taken by rowSource.Reset? @arjunravinarayan @asubiotto

rowSource.Reset is trivial (it sets an index). I doubt the b.Fatal ever fired. I'd move the placement of b.Run back in this PR and let @arjunravinarayan and @asubiotto sort it out in a follow-on.

---

Comments from Reviewable

[tamird]

Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

rowSource.Reset is trivial (it sets an index). I doubt the b.Fatal ever fired. I'd move the placement of b.Run back in this PR and let @arjunravinarayan and @asubiotto sort it out in a follow-on.

I went with a more explicit call to b.ResetTimer(); PTAL.

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

We're not consistent in capitalization for these "keys". Does that matter?

Ping.

---

Comments from Reviewable

[tamird]

Ah sorry, missed that. Doesn't matter.

…

On Jul 5, 2017 09:17, "Peter Mattis" ***@***.***> wrote: Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. ------------------------------ *pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 <https://reviewable.io:443/reviews/cockroachdb/cockroach/16830#-Ko-d0i5nSF_8_auaLiX:-KoHtLsqkmCyvfA3igz0:b96mcoh> (raw file <https://github.com/cockroachdb/cockroach/blob/fe610d56a6cb3e108591a09d9349e970241815e4/pkg/sql/distsqlrun/sorter_test.go#L245>):* We're not consistent in capitalization for these "keys". Does that matter? Ping. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/16830>* — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16830 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPPj_NWIucWEnzDRq3kK_pW91b14Zks5sK4zvgaJpZM4OLGQ5> .

[rjnn]

Skimmed, new convention looks fine.
Did not look closely.

---

Reviewed 1 of 9 files at r2, 3 of 9 files at r3.
Review status: 3 of 9 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

We're not consistent in capitalization for these "keys". Does that matter?

Ping.

I'll let @asubiotto authoritatively answer this.

---

Comments from Reviewable

[asubiotto]

Might be nice to make capitalization of the keys consistent but I have no strong feelings about it.

---

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

---

pkg/sql/distsqlrun/sorter_test.go, line 245 at r1 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

I'll let @asubiotto authoritatively answer this.

This call to ResetTimer is fine. I don't think there's a need to have this change in a separate PR.

---

Comments from Reviewable