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

sql: regression on EndToEnd/ored-preds #89547

Closed
yuzefovich opened this issue Oct 6, 2022 · 5 comments
Closed

sql: regression on EndToEnd/ored-preds #89547

yuzefovich opened this issue Oct 6, 2022 · 5 comments
Labels
C-investigation Further steps needed to qualify. C-label will change. T-sql-queries SQL Queries Team

Comments

@yuzefovich
Copy link
Member

yuzefovich commented Oct 6, 2022

Consider the following comparison between 22.1(edc5865) and 22.2(385bf90) of BenchmarkEndToEnd from pkg/sql/opt/bench:

EndToEnd/ored-preds-100/Simple-24                   2.53ms ± 1%    2.86ms ± 1%  +13.11%  (p=0.000 n=10+10)
EndToEnd/ored-preds-100/Prepared-24                 1.33ms ± 1%    1.47ms ± 1%  +10.92%  (p=0.000 n=10+8)
EndToEnd/ored-preds-using-params-100/Simple-24      4.35ms ± 1%    4.92ms ± 1%  +12.98%  (p=0.000 n=9+10)
EndToEnd/ored-preds-using-params-100/Prepared-24    2.85ms ± 1%    3.15ms ± 1%  +10.62%  (p=0.000 n=10+10)

(Full comparison is available here. The comparison was made with the following diff on 22.1 SHA:

--- a/pkg/util/goschedstats/runtime_go1.17.go
+++ b/pkg/util/goschedstats/runtime_go1.17.go
@@ -12,8 +12,8 @@
 // and go1.18. Before allowing newer versions, please check that the structures
 // still match with those in go/src/runtime.
 
-//go:build gc && go1.17 && !go1.19
-// +build gc,go1.17,!go1.19
+//go:build gc && go1.17 && !go1.20
+// +build gc,go1.17,!go1.20
 
 package goschedstats

to allow usage of go 1.19. The comparison was made with ./scripts/bench that used make under the hood with -benchtime=3s.)

Informs #87685. The regression doesn't seem that severe, so not marking it as a GA-blocker.

Jira issue: CRDB-20298

@yuzefovich yuzefovich added the C-investigation Further steps needed to qualify. C-label will change. label Oct 6, 2022
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Oct 6, 2022
@mgartner
Copy link
Collaborator

We should try to get to the bottom of this, but I think this is a pretty minor regression with a very specific edge-case, so it shouldn't block the release.

@mgartner
Copy link
Collaborator

The regressions are more mild for me when I run on a GCE worker:

name                                              old time/op  new time/op  delta
EndToEnd/ored-preds-100/Simple-24                 2.46ms ± 0%  2.65ms ± 1%  +7.69%  (p=0.016 n=4+5)
EndToEnd/ored-preds-100/Prepared-24               1.22ms ± 1%  1.29ms ± 0%  +5.71%  (p=0.008 n=5+5)
EndToEnd/ored-preds-using-params-100/Simple-24    4.16ms ± 1%  4.49ms ± 2%  +7.92%  (p=0.008 n=5+5)
EndToEnd/ored-preds-using-params-100/Prepared-24  2.77ms ± 0%  2.89ms ± 0%  +4.51%  (p=0.008 n=5+5)

I'l try to bisect to a commit now.

@mgartner
Copy link
Collaborator

I was unable to easily bisect this. On many of the commits between the two commits listed in the description, I'm unable to run benchmarks - make spins forever.

@yuzefovich
Copy link
Member Author

I also ran it on the gceworker with go 1.19.1.

@mgartner
Copy link
Collaborator

mgartner commented Dec 8, 2022

I'm going to close this. This regression seems very minor and the regressed benchmark is very contrived. We've already made significant improvements to this benchmark in the past, which dwarf this regression.

@mgartner mgartner closed this as completed Dec 8, 2022
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-investigation Further steps needed to qualify. C-label will change. T-sql-queries SQL Queries Team
Projects
Archived in project
Development

No branches or pull requests

2 participants