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

exec: add "auto" vectorize setting and make it default #38777

Merged
merged 2 commits into from
Aug 2, 2019

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Jul 9, 2019

First commit renames Vectorize to VectorizeMode and removes
"experimental_" prefix of vectorize setting.

Second commit adds a fourth option "auto" to vectorize execution mode setting
which plans the queries containing only streaming operators via the
vectorized engine. This option is made a default one. Other options
have been renamed: "on" -> "experimental_on" and "always" ->
"experimental_always" to highlight for users the risk of using them.

Fixes: #38620.
Fixes: #38920.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rafiss rafiss 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 (waiting on @asubiotto, @jordanlewis, and @solongordon)


pkg/sql/exec_util.go, line 157 at r3 (raw file):

		int64(sessiondata.VectorizeOff):       "off",
		int64(sessiondata.VectorizeOn):        "on",
		int64(sessiondata.VectorizeAlways):    "always",

i think one of the things we had discussed was to rename the "always" value to something like "experimental_on" (and maybe something similar for the current "on" setting?) so that it's more clear to users that it's likely broken. i'm not sure if we reached a firm conclusion, but just wondering if you had considered that.

Copy link
Member Author

@yuzefovich yuzefovich 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 (waiting on @asubiotto, @jordanlewis, and @solongordon)


pkg/sql/exec_util.go, line 157 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think one of the things we had discussed was to rename the "always" value to something like "experimental_on" (and maybe something similar for the current "on" setting?) so that it's more clear to users that it's likely broken. i'm not sure if we reached a firm conclusion, but just wondering if you had considered that.

I was actually hoping to reach a consensus about the naming while reviewing this PR, so thanks for starting up the discussion :)

I agree with your point, and I think I'm leaning towards on -> experimental_all and always -> experimental_forced, but I'm definitely open to suggestions so that it's clear that both can be broken and that choosing the latter sounds harsh.

@yuzefovich yuzefovich force-pushed the vec-setting branch 2 times, most recently from b0ca931 to cc555b2 Compare July 9, 2019 23:14
@yuzefovich yuzefovich added the do-not-merge bors won't merge a PR with this label. label Jul 9, 2019
Copy link
Contributor

@solongordon solongordon 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 (waiting on @asubiotto, @jordanlewis, and @solongordon)


pkg/sql/exec_util.go, line 157 at r3 (raw file):

Previously, yuzefovich wrote…

I was actually hoping to reach a consensus about the naming while reviewing this PR, so thanks for starting up the discussion :)

I agree with your point, and I think I'm leaning towards on -> experimental_all and always -> experimental_forced, but I'm definitely open to suggestions so that it's clear that both can be broken and that choosing the latter sounds harsh.

I like that, though it might also be nice to mirror the distsql setting's options:

const (
// DistSQLOff means that we never distribute queries.
DistSQLOff DistSQLExecMode = iota
// DistSQLAuto means that we automatically decide on a case-by-case basis if
// we distribute queries.
DistSQLAuto
// DistSQLOn means that we distribute queries that are supported.
DistSQLOn
// DistSQLAlways means that we only distribute; unsupported queries fail.
DistSQLAlways
)

So we could do off, auto, experimental_on, and experimental_always

Copy link
Contributor

@solongordon solongordon 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 (waiting on @asubiotto and @jordanlewis)


pkg/sql/exec_util.go, line 157 at r3 (raw file):

Previously, solongordon (Solon) wrote…

I like that, though it might also be nice to mirror the distsql setting's options:

const (
// DistSQLOff means that we never distribute queries.
DistSQLOff DistSQLExecMode = iota
// DistSQLAuto means that we automatically decide on a case-by-case basis if
// we distribute queries.
DistSQLAuto
// DistSQLOn means that we distribute queries that are supported.
DistSQLOn
// DistSQLAlways means that we only distribute; unsupported queries fail.
DistSQLAlways
)

So we could do off, auto, experimental_on, and experimental_always

Aside from just using consistent language, I think it would be nice to use auto rather than streaming because later we might want to start gradually enabling non-streaming operators by default. So it could be nice to have a more generic term like "auto" which represents everything we're comfortable enabling by default.

@yuzefovich yuzefovich force-pushed the vec-setting branch 13 times, most recently from 9bb4e3a to b174338 Compare July 17, 2019 17:33
@yuzefovich yuzefovich force-pushed the vec-setting branch 9 times, most recently from 8db9b23 to effc078 Compare August 2, 2019 15:05
Renames Vectorize to VectorizeMode and removes "experimental_"
prefix.

Release note: None
@yuzefovich yuzefovich force-pushed the vec-setting branch 5 times, most recently from 4631246 to fa5f826 Compare August 2, 2019 17:54
@yuzefovich yuzefovich removed the do-not-merge bors won't merge a PR with this label. label Aug 2, 2019
@yuzefovich
Copy link
Member Author

Alright, finally this is RFAL.

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.

:lgtm_strong: let's get this baby in!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)

@asubiotto
Copy link
Contributor

Should we wait until Monday to enable this so that we don't feel pressured to address any fallout?

@jordanlewis
Copy link
Member

I think I'd prefer to see this go in so that the nightlies can fail over the weekend and we'll have a fresh crop of stuff to look in by Monday :) Just don't check your work email til next week.

Adds a fourth option "auto" to vectorize execution mode setting
which plans the queries containing only streaming operators via the
vectorized engine. This option is made a default one. Other options
have been renamed: "on" -> "experimental_on" and "always" ->
"experimental_always" to highlight for users the risk of using them.

Release note: None
@yuzefovich
Copy link
Member Author

I ran KV99 tests again:

name             old ops/s   new ops/s   delta
KV99-throughput  21.1k ± 3%  17.5k ± 1%  -17.14%  (p=0.008 n=5+5)

name             old ms/s    new ms/s    delta
KV99-P50          16.7 ± 7%   19.9 ± 0%  +19.16%  (p=0.008 n=5+5)
KV99-Avg          5.12 ± 2%   6.00 ± 0%  +17.19%  (p=0.016 n=5+4)

so a slightly bigger performance hit than we observed previously (probably because of the enhanced SupportsVectorized check).

I agree with Jordan, let's merge it now, and we will see the fresh fallout on Monday.

Thanks for the review and the inputs!

bors r+

craig bot pushed a commit that referenced this pull request Aug 2, 2019
38777: exec: add "auto" vectorize setting and make it default r=yuzefovich a=yuzefovich

First commit renames Vectorize to VectorizeMode and removes
"experimental_" prefix of vectorize setting.

Second commit adds a fourth option "auto" to vectorize execution mode setting
which plans the queries containing only streaming operators via the
vectorized engine. This option is made a default one. Other options
have been renamed: "on" -> "experimental_on" and "always" ->
"experimental_always" to highlight for users the risk of using them.

Fixes: #38620.
Fixes: #38920.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Aug 2, 2019

Build succeeded

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.

exec: tracking issue for turning on "auto" by default exec: finalize vectorize setting name and behavior
6 participants