-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
opt: add FK benchmarks #39804
opt: add FK benchmarks #39804
Conversation
Adding simple insert benchmarks comparing no FKs with the old FK path and the new one. The results are below. Note that we haven't yet optimized the constant input case (we can remove the buffer / scan nodes). Command line: ``` GOMAXPROCS=1 make bench PKG=./pkg/sql/opt/bench BENCHTIMEOUT=30m BENCHES='BenchmarkFKInsert/' TESTFLAGS='-logtostderr NONE -benchtime=20000x -count 5' ``` ``` name time/op FKInsert/SingleRow/None 609µs ± 4% FKInsert/SingleRow/Old 676µs ± 5% FKInsert/SingleRow/New 961µs ± 1% FKInsert/MultiRowSingleParent/None 1.37ms ± 1% FKInsert/MultiRowSingleParent/Old 2.14ms ± 1% FKInsert/MultiRowSingleParent/New 2.11ms ± 2% FKInsert/MultiRowMultiParent/None 1.62ms ± 2% FKInsert/MultiRowMultiParent/Old 2.89ms ± 1% FKInsert/MultiRowMultiParent/New 2.97ms ± 1% ``` Release note: None
So the % changes means we are 4% faster? |
No, the % here is the uncertainty bounds of the benchmark. The included numbers tell you how long each op takes in a vacuum. You can compare them by looking at the Old/New numbers it looks like, right Radu? |
@awoods187 The new path is slower in this benchmark (but there are still some optimizations to come; also this benchmark will be useful to find any unexpected performance issues in the implementation but is not really reflective of what would happen on a real cluster). I don't think it's productive to discuss things like this per-PR. We will collect more useful perf numbers and we'll be sure to run them by you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @justinj)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty exciting to see no gigantic regressions even in this preliminary benchmark!
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @justinj)
TFTRs! bors r+ |
Build failed |
bors r+ |
39804: opt: add FK benchmarks r=RaduBerinde a=RaduBerinde Adding simple insert benchmarks comparing no FKs with the old FK path and the new one. The results are below. Note that we haven't yet optimized the constant input case (we can remove the buffer / scan nodes). Command line: ``` GOMAXPROCS=1 make bench PKG=./pkg/sql/opt/bench BENCHTIMEOUT=30m BENCHES='BenchmarkFKInsert/' TESTFLAGS='-logtostderr NONE -benchtime=20000x -count 5' ``` ``` name time/op FKInsert/SingleRow/None 609µs ± 4% FKInsert/SingleRow/Old 676µs ± 5% FKInsert/SingleRow/New 961µs ± 1% FKInsert/MultiRowSingleParent/None 1.37ms ± 1% FKInsert/MultiRowSingleParent/Old 2.14ms ± 1% FKInsert/MultiRowSingleParent/New 2.11ms ± 2% FKInsert/MultiRowMultiParent/None 1.62ms ± 2% FKInsert/MultiRowMultiParent/Old 2.89ms ± 1% FKInsert/MultiRowMultiParent/New 2.97ms ± 1% ``` Release note: None 39818: opt: nil presentation should not equal the 0-column presentation r=RaduBerinde a=RaduBerinde #### sql: enhance apply execbuilder error We have seen a series of bugs where the execbuilder hits an assertion error inside Apply. This change adds a detail to the error containing the formatted opt tree. Release note: None #### opt: nil presentation should not equal the 0-column presentation The nil presentation means that we want all columns in no particular order, whereas the 0-column presentation means that we don't need any of the columns (e.g. EXISTS subqueries). Only the nil presentation is `Any()`. But `Equals()` was incorrectly treating these as equal. This confused the interner and a nil presentation became a 0-column presentation which led to some internal errors in Apply. For completeness, we also fix `HashPhysProps` to differentiate between these two, but note that this isn't required for correctness. Fixes #39435. Release note (bug fix): Fixed internal errors generated during execution of some complicated cases of correlated subqueries. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Build succeeded |
Adding simple insert benchmarks comparing no FKs with the old FK path
and the new one.
The results are below. Note that we haven't yet optimized the constant
input case (we can remove the buffer / scan nodes).
Command line:
Release note: None