Skip to content

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

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/bumpApd3.1.0
Feb 2, 2022
Merged

vendor: bump cockroachdb/apd to v3.1.0, speed up decimal division#75770
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/bumpApd3.1.0

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb 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
Copy Markdown
Member

This change is Reviewable

@nvb nvb force-pushed the nvanbenschoten/bumpApd3.1.0 branch 2 times, most recently from 27ef174 to 2abb651 Compare February 1, 2022 05:33
@nvb nvb 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
@nvb nvb requested a review from yuzefovich February 1, 2022 15:04
@nvb nvb marked this pull request as ready for review February 1, 2022 15:04
@nvb nvb requested review from a team as code owners February 1, 2022 15:04
Copy link
Copy Markdown
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`.
@nvb nvb force-pushed the nvanbenschoten/bumpApd3.1.0 branch from 2abb651 to a134352 Compare February 2, 2022 00:25
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Feb 2, 2022

TFTR!

bors r+

@craig craig bot merged commit 63988a4 into cockroachdb:master Feb 2, 2022
@craig
Copy link
Copy Markdown
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