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

apd: avoid function calls on hot paths #112

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Jan 17, 2022

This PR includes a pair of minor optimizations that avoid function calls on arithmetic hot paths.

short-circuit in Context.goError

This commit adds a fast-path for the common case in Context.goError that avoids the function call to Condition.GoError. Context.goError is inlined in callers, so this avoids any function call in the common case.

inline Context.round

This commit reworks the call to Context.round to allow for mid-stack function inlining. To do this, we remove the conditional call to one of two functions, which is considered too complex to inline.


name                 old time/op    new time/op    delta
GDA/minus-10           6.43µs ± 5%    6.11µs ± 5%  -5.11%  (p=0.000 n=25+25)
GDA/subtract-10        85.3µs ± 5%    81.2µs ± 2%  -4.81%  (p=0.000 n=25+19)
GDA/abs-10             4.76µs ± 5%    4.58µs ± 6%  -3.87%  (p=0.000 n=25+25)
GDA/tointegral-10      16.7µs ± 6%    16.1µs ± 5%  -3.77%  (p=0.000 n=25+20)
GDA/remainder-10       28.1µs ± 6%    27.1µs ± 6%  -3.70%  (p=0.000 n=24+25)
GDA/reduce-10          10.8µs ± 5%    10.5µs ± 6%  -3.19%  (p=0.000 n=25+25)
GDA/squareroot-10      12.6ms ± 5%    12.2ms ± 1%  -2.97%  (p=0.000 n=19+19)
GDA/divideint-10       13.9µs ± 5%    13.6µs ± 6%  -2.21%  (p=0.000 n=25+25)
GDA/tointegralx-10     17.0µs ± 5%    16.7µs ± 6%  -2.03%  (p=0.000 n=25+21)
GDA/powersqrt-10        194ms ± 4%     191ms ± 0%  -1.91%  (p=0.000 n=25+19)
GDA/quantize-10        71.2µs ± 5%    70.0µs ± 5%  -1.80%  (p=0.000 n=25+25)
GDA/plus-10            33.8µs ± 6%    33.2µs ± 6%  -1.59%  (p=0.000 n=22+20)
GDA/rounding-10         254µs ± 0%     250µs ± 0%  -1.56%  (p=0.000 n=19+19)
GDA/power-10            215ms ± 5%     212ms ± 0%  -1.30%  (p=0.000 n=24+19)
GDA/randoms-10         1.72ms ± 0%    1.70ms ± 0%  -0.93%  (p=0.000 n=19+19)
GDA/cuberoot-apd-10    1.80ms ± 4%    1.79ms ± 4%  -0.61%  (p=0.001 n=25+25)
GDA/log10-10            102ms ± 6%     102ms ± 0%  -0.43%  (p=0.024 n=20+19)
GDA/divide-10           234µs ± 5%     233µs ± 5%  -0.42%  (p=0.001 n=25+25)
GDA/ln-10              78.7ms ± 0%    78.5ms ± 0%  -0.24%  (p=0.000 n=19+19)
GDA/base-10             112µs ± 1%     112µs ± 2%    ~     (p=0.745 n=19+24)
GDA/compare-10         26.0µs ± 6%    26.1µs ± 6%    ~     (p=0.291 n=25+25)
GDA/comparetotal-10    24.3µs ± 6%    24.3µs ± 6%    ~     (p=0.473 n=25+25)
GDA/exp-10              117ms ± 3%     118ms ± 5%    ~     (p=0.352 n=19+25)
GDA/multiply-10        50.9µs ± 1%    51.1µs ± 5%    ~     (p=0.053 n=19+25)
GDA/add-10              556µs ± 1%     560µs ± 6%  +0.68%  (p=0.015 n=19+25)

note: the GDA/add test suite is heavily skewed towards extreme values and edge cases. GDA/subtract is a more representative test suite for common addition and subtraction operations.

This commit adds a fast-path for the common case in `Context.goError`
that avoids the function call to `Condition.GoError`. `Context.goError`
is inlined in callers, so this avoids any function call in the common
case.

The commit also adds two addtional `gcassert:inline` directives.
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 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nvanbenschoten)


-- commits, line 15 at r2:
nit: probably s/conditionally/conditional/.


round.go, line 70 at r2 (raw file):

	if disableIfPrecisionZero && c.Precision == 0 {
		// Rounding has been disabled.
		res |= d.setExponent(c, nd, res, int64(d.Exponent))

nit: why do we perform logical OR and not just return d.setExponent(c, nd, 0, int64(d.Exponent))?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

This commit reworks the call to `Context.round` to allow for mid-stack
function inlining. To do this, we remove the conditional call to one
of two functions, which is considered too complex to inline.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/inlineGoError branch from 23eba28 to c932774 Compare January 29, 2022 20:12
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuzefovich)


-- commits, line 15 at r2:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: probably s/conditionally/conditional/.

Done.


round.go, line 70 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: why do we perform logical OR and not just return d.setExponent(c, nd, 0, int64(d.Exponent))?

The idea was to mirror the structure of the other logic in this function more closely and not assume that res is 0 at this point in the code. But I agree that it's unnecessary and not worth the confusion. Changed.

@nvanbenschoten nvanbenschoten merged commit 86b4932 into cockroachdb:master Jan 29, 2022
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/inlineGoError branch January 29, 2022 20:21
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Jan 29, 2022
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Jan 29, 2022
75710: vendor: bump cockroachdb/apd to v3.0.1 r=nvanbenschoten a=nvanbenschoten

Picks up cockroachdb/apd#112.

Co-authored-by: Nathan VanBenschoten <[email protected]>
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