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.

Sign up for GitHub

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

Jump to bottom

tscache: fix perf regression by avoiding expensive error checks #49324

Closed
wants to merge 1 commit into from
Closed

tscache: fix perf regression by avoiding expensive error checks #49324

wants to merge 1 commit into from

Conversation

asubiotto
Copy link
Contributor

This code was previously using errors.Is in cases where it can be avoided by
first checking if err != nil.

Release note: None (regression was not part of a release)

Benchmark results here

@asubiotto
tscache: fix perf regression by avoiding expensive error checks
1d4ae34 
This code was previously using errors.Is in cases where it can be avoided by
first checking if err != nil.

Release note: None (regression was not part of a release)
@asubiotto asubiotto requested a review from knz May 20, 2020 13:44
@cockroach-teamcity
Copy link
Member

This change is Reviewable

knz
knz reviewed May 20, 2020
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up.
I am worried about the fact this is required -- the case where err == nil should be fast in errors.Is already. If it's not, that's a bug in errors.Is.

I'll have a look.

@asubiotto asubiotto mentioned this pull request May 20, 2020
Merged
@knz
Copy link
Contributor

knz commented May 20, 2020

Here's your fix: cockroachdb/errors#33

@knz knz mentioned this pull request May 21, 2020
Merged
@knz
Copy link
Contributor

knz commented May 21, 2020 •
edited
Loading

I think this will do it: #49356

craig bot pushed a commit that referenced this pull request May 21, 2020
@knz
Merge #49356
302479d 
49356: deps: bump cockroachdb/errors r=tbg a=knz

This fixes a perf regression introduced in d88ac05.
(Regression found by @asubiotto )

Supersedes #49324

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@knz
Copy link
Contributor

knz commented May 21, 2020

can you try your benchmark again now that #49356 is merged?

@asubiotto
Copy link
Contributor Author

Yup! Looks good:

IntervalSklAdd/size_1-24              6.52µs ± 5%    4.08µs ± 3%  -37.34%  (p=0.000 n=10+10)
IntervalSklAdd/size_10-24             10.6µs ± 6%     6.0µs ± 4%  -43.84%  (p=0.000 n=10+10)
IntervalSklAdd/size_100-24            10.7µs ± 4%     5.9µs ± 2%  -44.61%  (p=0.000 n=10+10)
IntervalSklAdd/size_1000-24           10.7µs ± 3%     6.1µs ± 2%  -43.29%  (p=0.000 n=9+9)
IntervalSklAdd/size_10000-24          11.1µs ± 2%     6.4µs ± 2%  -41.74%  (p=0.000 n=10+10)
IntervalSklAdd/size_100000-24         12.9µs ± 3%     8.1µs ± 3%  -37.26%  (p=0.000 n=10+10)
IntervalSklAdd/size_1000000-24        17.8µs ± 9%    12.2µs ± 4%  -31.48%  (p=0.000 n=10+9)
IntervalSklAdd/size_10000000-24       31.6µs ±10%    24.4µs ± 4%  -22.86%  (p=0.000 n=10+10)
IntervalSklAdd/size_100000000-24      65.1µs ±11%    56.6µs ± 7%  -13.08%  (p=0.000 n=10+9)
IntervalSklAddAndLookup/frac_0-24     4.60µs ± 2%    3.58µs ± 4%  -22.34%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_1-24     4.22µs ± 4%    3.32µs ± 2%  -21.32%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_2-24     3.93µs ± 2%    3.06µs ± 2%  -22.17%  (p=0.000 n=9+9)
IntervalSklAddAndLookup/frac_3-24     3.65µs ± 3%    2.84µs ± 2%  -22.18%  (p=0.000 n=10+9)
IntervalSklAddAndLookup/frac_4-24     3.40µs ± 2%    2.62µs ± 2%  -22.98%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_5-24     3.17µs ± 3%    2.40µs ± 4%  -24.23%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_6-24     2.85µs ± 3%    2.16µs ± 2%  -24.05%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_7-24     2.55µs ± 3%    1.91µs ± 2%  -24.91%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_8-24     2.25µs ± 1%    1.65µs ± 3%  -26.73%  (p=0.000 n=9+10)
IntervalSklAddAndLookup/frac_9-24     1.96µs ± 3%    1.44µs ± 6%  -26.34%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_10-24    1.71µs ± 3%    1.28µs ± 1%  -25.03%  (p=0.000 n=9+10)

name                                old alloc/op   new alloc/op   delta
IntervalSklAdd/size_1-24                702B ±19%      204B ±29%  -70.97%  (p=0.000 n=10+10)
IntervalSklAdd/size_10-24             1.10kB ± 0%    0.14kB ± 0%  -86.94%  (p=0.000 n=9+8)
IntervalSklAdd/size_100-24            1.10kB ± 0%    0.14kB ± 0%  -86.94%  (p=0.000 n=10+10)
IntervalSklAdd/size_1000-24           1.10kB ± 0%    0.14kB ± 0%  -86.94%  (p=0.000 n=9+9)
IntervalSklAdd/size_10000-24          1.10kB ± 0%    0.22kB ±56%  -80.34%  (p=0.000 n=8+10)
IntervalSklAdd/size_100000-24         1.30kB ±15%    0.37kB ±64%  -71.75%  (p=0.000 n=10+10)
IntervalSklAdd/size_1000000-24        1.70kB ±45%    0.70kB ±79%  -59.07%  (p=0.001 n=10+10)
IntervalSklAdd/size_10000000-24       3.13kB ±39%    1.60kB ±91%  -48.85%  (p=0.004 n=10+10)
IntervalSklAdd/size_100000000-24      5.94kB ±54%    3.71kB ±96%  -37.61%  (p=0.043 n=10+10)
IntervalSklAddAndLookup/frac_0-24       606B ±64%      503B ±73%     ~     (p=0.269 n=10+10)
IntervalSklAddAndLookup/frac_1-24       306B ± 0%      126B ± 1%  -58.99%  (p=0.000 n=10+9)
IntervalSklAddAndLookup/frac_2-24       285B ± 0%      116B ± 0%  -59.36%  (p=0.000 n=10+8)
IntervalSklAddAndLookup/frac_3-24       264B ± 0%      106B ± 0%  -59.90%  (p=0.000 n=9+8)
IntervalSklAddAndLookup/frac_4-24       243B ± 0%       96B ± 1%  -60.71%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_5-24       223B ± 1%       86B ± 1%  -61.55%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_6-24       201B ± 0%       76B ± 1%  -62.47%  (p=0.000 n=10+9)
IntervalSklAddAndLookup/frac_7-24       180B ± 0%       66B ± 1%  -63.67%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_8-24       159B ± 0%       55B ± 1%  -65.24%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_9-24       138B ± 1%       46B ± 1%  -66.98%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_10-24      116B ± 0%       36B ± 0%  -68.97%  (p=0.000 n=10+10)

name                                old allocs/op  new allocs/op  delta
IntervalSklAdd/size_1-24                17.0 ± 0%       6.0 ± 0%  -64.71%  (p=0.000 n=10+10)
IntervalSklAdd/size_10-24               29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_100-24              29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_1000-24             29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_10000-24            29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_100000-24           29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_1000000-24          29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_10000000-24         29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAdd/size_100000000-24        29.0 ± 0%       6.0 ± 0%  -79.31%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_0-24       10.0 ± 0%       6.0 ± 0%  -40.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_1-24       10.0 ± 0%       5.0 ± 0%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_2-24       9.00 ± 0%      5.00 ± 0%  -44.44%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_3-24       8.00 ± 0%      4.00 ± 0%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_4-24       8.00 ± 0%      4.00 ± 0%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_5-24       7.00 ± 0%      3.50 ±14%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_6-24       6.00 ± 0%      3.00 ± 0%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_7-24       6.00 ± 0%      3.00 ± 0%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_8-24       5.00 ± 0%      2.00 ± 0%  -60.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_9-24       4.00 ± 0%      2.00 ± 0%  -50.00%  (p=0.000 n=10+10)
IntervalSklAddAndLookup/frac_10-24      4.00 ± 0%      2.00 ± 0%  -50.00%  (p=0.000 n=10+10)

@asubiotto asubiotto closed this May 21, 2020
@asubiotto asubiotto deleted the tspf branch May 21, 2020 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asubiotto @cockroach-teamcity @knz