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

storage: MVCCGetWithValueHeader not inlined in MVCCGet #93154

Closed
tbg opened this issue Dec 6, 2022 · 3 comments
Closed

storage: MVCCGetWithValueHeader not inlined in MVCCGet #93154

tbg opened this issue Dec 6, 2022 · 3 comments
Assignees
@tbg
Labels
branch-master Failures and bugs on the master branch. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker

Comments

@tbg
Copy link
Member

tbg commented Dec 6, 2022 •
edited by cockroach-jira-scripts
Loading

Describe the problem

In #89477, we're introducing MVCCGetWithValueHeader. Unfortunately, this is called from MVCCGet and the call there is not inlined, which could have a performance penalty since it's a hot code path. We should check and potentially optimize further.

To Reproduce

Add // gcassert:inline to MVCCValueHeader and run the lints (unfortunately can't run them locally)

Jira issue: CRDB-22190
@tbg tbg added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv-replication labels Dec 6, 2022
@blathers-crl
Copy link

blathers-crl bot commented Dec 6, 2022

cc @cockroachdb/replication

tbg added a commit to tbg/cockroach that referenced this issue Dec 7, 2022
@tbg
storage: compromise on gcassert for MVCCGetWithValueHeader
19f2fad 
See cockroachdb#93154.

Release note: None
@erikgrinaker erikgrinaker added C-performance Perf of queries or internals. Solution not expected to change functional behavior. and removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Jan 9, 2023
@exalate-issue-sync exalate-issue-sync bot added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed C-performance Perf of queries or internals. Solution not expected to change functional behavior. labels Jan 9, 2023
@blathers-crl
Copy link

blathers-crl bot commented Mar 21, 2023

Hi @erikgrinaker, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@erikgrinaker erikgrinaker added the branch-master Failures and bugs on the master branch. label Mar 21, 2023
@tbg
Copy link
Member Author

tbg commented Mar 21, 2023 

benchdiff -n 1b4947bf4e1036a7c55996d2cb6ab815254a3c8c -r BenchmarkMVCCGet_Pebble --post-checkout './dev gen go' --previous-run 2023-03-21T13_02_01Z ./pkg/storage

(new = PR that introduced MVCCGetWithValueHeader, 1b4947b)

We don't seem to have regressed here at all, rather we got a tiny bit faster though that might be noise. Numbers are from an idle gceworker.

Details 
name                                                                     old time/op    new time/op    delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=1-24        11.2µs ± 1%    10.9µs ± 1%  -2.67%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24         6.07µs ± 1%    5.94µs ± 1%  -2.11%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=0-24        6.65µs ± 1%    6.53µs ± 1%  -1.83%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24         3.93µs ± 1%    3.87µs ± 1%  -1.55%  (p=0.001 n=10+9)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=100-24       120µs ± 2%     120µs ± 1%    ~     (p=0.780 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=0-24       26.5µs ± 2%    26.4µs ± 2%    ~     (p=0.645 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=1-24       33.8µs ± 4%    33.5µs ± 5%    ~     (p=0.356 n=9+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=100-24      105µs ± 3%     106µs ± 2%    ~     (p=0.182 n=10+9)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=0-24       111µs ± 1%     110µs ± 4%    ~     (p=0.434 n=9+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=1-24       122µs ± 2%     121µs ± 2%    ~     (p=0.113 n=9+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=100-24     200µs ± 4%     201µs ± 3%    ~     (p=0.796 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=100-24       70.0µs ± 1%    69.8µs ± 1%    ~     (p=0.436 n=9+9)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=0-24        22.6µs ± 3%    22.3µs ± 1%    ~     (p=0.053 n=10+9)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=1-24        27.8µs ± 3%    27.5µs ± 3%    ~     (p=0.074 n=9+8)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=100-24      96.2µs ± 2%    95.8µs ± 2%    ~     (p=0.549 n=10+9)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24        108µs ± 2%     108µs ± 2%    ~     (p=0.715 n=9+9)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=1-24        117µs ± 3%     117µs ± 6%    ~     (p=0.829 n=8+10)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=100-24      193µs ± 3%     191µs ± 4%    ~     (p=0.258 n=9+9)

name                                                                     old speed      new speed      delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=1-24       713kB/s ± 1%   733kB/s ± 1%  +2.85%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24       1.32MB/s ± 1%  1.35MB/s ± 1%  +2.72%  (p=0.000 n=10+7)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=0-24      1.20MB/s ± 1%  1.22MB/s ± 1%  +1.87%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24       2.03MB/s ± 1%  2.07MB/s ± 1%  +1.55%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=100-24    70.0kB/s ± 0%  70.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=0-24      300kB/s ± 0%   305kB/s ± 5%    ~     (p=0.137 n=8+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=1-24      238kB/s ± 5%   238kB/s ± 5%    ~     (p=1.117 n=9+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=100-24   80.0kB/s ± 0%  80.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=0-24    70.0kB/s ± 0%  70.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=1-24    70.0kB/s ± 0%  70.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=100-24  40.0kB/s ± 0%  40.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=100-24      110kB/s ± 0%   113kB/s ± 6%    ~     (p=0.248 n=9+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=0-24       355kB/s ± 4%   360kB/s ± 0%    ~     (p=0.176 n=10+7)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=1-24       290kB/s ± 0%   290kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=100-24    80.0kB/s ± 0%  80.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24     70.0kB/s ± 0%  70.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=1-24     70.0kB/s ± 0%  70.0kB/s ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=100-24   40.0kB/s ± 0%  40.0kB/s ± 0%    ~     (all equal)

name                                                                     old alloc/op   new alloc/op   delta
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24           121B ± 0%      121B ± 0%  -0.33%  (p=0.046 n=8+10)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=0-24          292B ± 0%      291B ± 0%  -0.21%  (p=0.003 n=8+10)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=1-24          476B ± 0%      476B ± 0%    ~     (p=0.087 n=10+10)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=100-24      22.9kB ± 1%    22.9kB ± 0%    ~     (p=0.661 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=0-24         367B ± 2%      365B ± 1%    ~     (p=0.476 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=1-24         648B ± 1%      646B ± 1%    ~     (p=0.432 n=9+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=100-24     23.2kB ± 1%    23.3kB ± 1%    ~     (p=0.063 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=0-24        944B ± 2%      978B ± 8%    ~     (p=0.092 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=1-24      1.28kB ± 3%    1.28kB ± 4%    ~     (p=0.971 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=100-24    23.9kB ± 2%    24.0kB ± 3%    ~     (p=0.631 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24           200B ± 0%      200B ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=100-24       21.1kB ± 0%    21.2kB ± 1%    ~     (p=0.119 n=10+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=0-24          160B ± 3%      159B ± 1%    ~     (p=0.180 n=10+9)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=1-24          287B ± 3%      284B ± 1%    ~     (p=0.102 n=9+8)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24         387B ± 7%      393B ± 8%    ~     (p=0.343 n=10+10)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=1-24         548B ± 7%      550B ± 6%    ~     (p=0.927 n=10+10)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=100-24     21.6kB ± 2%    21.8kB ± 1%    ~     (p=0.165 n=10+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=100-24      21.6kB ± 0%    21.7kB ± 1%  +0.38%  (p=0.017 n=10+9)

name                                                                     old allocs/op  new allocs/op  delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=0-24          4.00 ± 0%      4.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=1-24          7.00 ± 0%      7.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=100-24        27.0 ± 0%      27.0 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=0-24         4.00 ± 0%      4.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=1-24         7.00 ± 0%      7.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=100-24       28.0 ± 0%      28.0 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=0-24        4.00 ± 0%      4.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=1-24        8.00 ± 0%      8.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8/numRangeKeys=100-24      29.0 ± 0%      29.0 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24           2.00 ± 0%      2.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24           2.00 ± 0%      2.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=100-24         21.0 ± 0%      21.0 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=0-24          2.00 ± 0%      2.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=1-24          2.00 ± 0%      2.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=100-24        22.0 ± 0%      22.0 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24         2.00 ± 0%      2.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=1-24         2.00 ± 0%      2.00 ± 0%    ~     (all equal)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=100-24       22.0 ± 0%      22.0 ± 0%    ~     (all equal)

@tbg tbg closed this as completed Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tbg tbg

Labels
branch-master Failures and bugs on the master branch. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@erikgrinaker @tbg