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

vendor: bump cockroachdb/apd to v3.1.0, speed up decimal division #75770

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Feb 1, 2022

Picks up two PRs that improved the performance of Quo, Sqrt, Cbrt, Exp, Ln, Log, and Pow:

Almost all of the testing changes here are due to the rounding behavior in cockroachdb/apd#115. This brings us closer to PG's behavior, but also creates a lot of noise in this diff. To verify that this noise wasn't hiding any correctness regressions caused by the rewrite of Context.Quo in the first PR, I created #75757, which only includes the first PR. #75757 passes CI with minimal testing changes. The testing changes that PR did require all have to do with trailing zeros, and most of them are replaced in this PR.

Release note (performance improvement): The performance of many DECIMAL arithmetic operators has been improved by as much as 60%. These operators include division (/), sqrt, cbrt, exp, ln, log, and pow.


Speedup on TPC-DS dataset

The TPC-DS dataset is full of decimal columns, so it's a good playground to test this change. Unfortunately, the variance in the runtime performance of the TPC-DS queries themselves is high (many queries varied by 30-40% per attempt), so it was hard to get signal out of them. Instead, I imported the TPC-DS dataset with a scale factor of 10 and ran some custom aggregation queries against the largest table (web_sales, row count = 7,197,566):

# q1
select sum(ws_wholesale_cost / ws_ext_list_price) from web_sales;

# q2
select sum(ws_wholesale_cost / ws_ext_list_price - sqrt(ws_net_paid_inc_tax)) from web_sales;

Here's the difference in runtime of these two queries before and after this change on an n2-standard-8 instance:

name              old s/op   new s/op   delta
TPC-DS/custom/q1  22.4 ± 0%   8.6 ± 0%  -61.33%  (p=0.002 n=6+6)
TPC-DS/custom/q2  91.4 ± 0%  32.1 ± 0%  -64.85%  (p=0.008 n=5+5)

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/bumpApd3.1.0 branch 2 times, most recently from 27ef174 to 2abb651 Compare February 1, 2022 05:33
@nvanbenschoten nvanbenschoten changed the title vendor: bump cockroachdb/apd to v3.1.0 vendor: bump cockroachdb/apd to v3.1.0, speed up decimal division Feb 1, 2022
@nvanbenschoten nvanbenschoten marked this pull request as ready for review February 1, 2022 15:04
@nvanbenschoten nvanbenschoten requested review from a team as code owners February 1, 2022 15:04
Copy link
Member

@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.

:lgtm:

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

Picks up two PRs that improved the performance of `Quo`, `Sqrt`,
`Cbrt`, `Exp`, `Ln`, `Log`, and `Pow`:
- cockroachdb/apd#114
- cockroachdb/apd#115

Almost all of the testing changes here are due to the rounding behavior
in cockroachdb/apd#115. This brings us closer
to PG's behavior, but also creates a lot of noise in this diff.

Release note (performance improvement): The performance of many DECIMAL
arithmetic operators has been improved by as much as 60%. These operators
include division (`/`), `sqrt`, `cbrt`, `exp`, `ln`, `log`, and `pow`.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/bumpApd3.1.0 branch from 2abb651 to a134352 Compare February 2, 2022 00:25
@nvanbenschoten
Copy link
Member Author

TFTR!

bors r+

@craig craig bot merged commit 63988a4 into cockroachdb:master Feb 2, 2022
@craig
Copy link
Contributor

craig bot commented Feb 2, 2022

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.

3 participants