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

kv: allocate Batch{Request|Response} header and {requests|responses} together #137108

Merged
merged 2 commits into from
Dec 11, 2024
Merged

kv: allocate Batch{Request|Response} header and {requests|responses} together #137108

merged 2 commits into from
Dec 11, 2024

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Dec 10, 2024 •
edited
Loading

These two commits avoid heap allocations in the common cases by allocating the BatchRequest and BatchResponse headers and their respective requests or responses together, in a single malloc.

Together, this avoids a pair of heap allocations per KV Batch operation.

name                                            old time/op    new time/op    delta
Sysbench/SQL/1node_remote/oltp_read_only-10       3.55ms ±17%    3.54ms ±16%    ~     (p=0.945 n=19+20)
Sysbench/SQL/1node_remote/oltp_write_only-10      2.13ms ±20%    2.13ms ±23%    ~     (p=0.531 n=19+20)
Sysbench/SQL/1node_remote/oltp_read_write-10      5.64ms ± 9%    5.77ms ±24%    ~     (p=0.988 n=18+19)
Sysbench/SQL/1node_remote/oltp_point_select-10     174µs ±10%     177µs ±19%    ~     (p=0.799 n=19+18)
Sysbench/KV/1node_remote/oltp_read_only-10         843µs ±17%     824µs ±15%    ~     (p=0.461 n=20+19)
Sysbench/KV/1node_remote/oltp_write_only-10        784µs ±14%     751µs ±24%    ~     (p=0.226 n=18+18)
Sysbench/KV/1node_remote/oltp_read_write-10       1.75ms ±15%    1.76ms ±21%    ~     (p=0.639 n=17+19)
Sysbench/KV/1node_remote/oltp_point_select-10     35.7µs ±16%    36.4µs ±25%    ~     (p=0.795 n=19+19)

name                                            old alloc/op   new alloc/op   delta
Sysbench/KV/1node_remote/oltp_point_select-10     6.57kB ± 0%    6.56kB ± 0%  -0.26%  (p=0.005 n=20+20)
Sysbench/SQL/1node_remote/oltp_read_only-10       1.15MB ± 1%    1.15MB ± 0%    ~     (p=0.191 n=19+19)
Sysbench/SQL/1node_remote/oltp_write_only-10       573kB ± 4%     573kB ± 4%    ~     (p=0.758 n=20+20)
Sysbench/SQL/1node_remote/oltp_read_write-10      1.64MB ± 1%    1.64MB ± 2%    ~     (p=0.771 n=19+20)
Sysbench/SQL/1node_remote/oltp_point_select-10    30.0kB ± 1%    30.1kB ± 1%    ~     (p=0.632 n=20+19)
Sysbench/KV/1node_remote/oltp_read_only-10         656kB ± 0%     655kB ± 0%    ~     (p=0.067 n=18+20)
Sysbench/KV/1node_remote/oltp_write_only-10        263kB ± 2%     263kB ± 2%    ~     (p=0.961 n=19+16)
Sysbench/KV/1node_remote/oltp_read_write-10        879kB ± 1%     880kB ± 1%    ~     (p=0.495 n=20+20)

name                                            old allocs/op  new allocs/op  delta
Sysbench/KV/1node_remote/oltp_point_select-10       61.0 ± 0%      59.0 ± 0%  -3.28%  (p=0.000 n=20+20)
Sysbench/KV/1node_remote/oltp_read_only-10         1.87k ± 0%     1.84k ± 0%  -1.50%  (p=0.000 n=17+17)
Sysbench/KV/1node_remote/oltp_read_write-10        3.46k ± 0%     3.42k ± 0%  -1.38%  (p=0.000 n=20+19)
Sysbench/KV/1node_remote/oltp_write_only-10        1.59k ± 0%     1.57k ± 0%  -1.32%  (p=0.000 n=20+18)
Sysbench/SQL/1node_remote/oltp_write_only-10       3.98k ± 1%     3.94k ± 1%  -1.00%  (p=0.000 n=20+20)
Sysbench/SQL/1node_remote/oltp_read_only-10        4.78k ± 1%     4.74k ± 1%  -0.76%  (p=0.000 n=19+19)
Sysbench/SQL/1node_remote/oltp_point_select-10       266 ± 1%       264 ± 1%  -0.64%  (p=0.003 n=20+19)
Sysbench/SQL/1node_remote/oltp_read_write-10       8.74k ± 0%     8.68k ± 1%  -0.60%  (p=0.000 n=19+18)

Epic: None
Release note: None

@nvanbenschoten nvanbenschoten added the o-perf-efficiency Related to performance efficiency label Dec 10, 2024
@nvanbenschoten nvanbenschoten requested a review from tbg December 10, 2024 18:36
@nvanbenschoten nvanbenschoten requested review from a team as code owners December 10, 2024 18:36
@nvanbenschoten nvanbenschoten requested review from xinhaoz, arjunmahishi and aa-joshi and removed request for a team December 10, 2024 18:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroachdb cockroachdb deleted a comment from blathers-crl bot Dec 10, 2024
RaduBerinde
RaduBerinde approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @tbg, and @xinhaoz)

@nvanbenschoten
kv: allocate BatchRequest header and first request together
34e3abf 
Avoids a heap allocation on each BatchStream RPC request.

```
name                                           old time/op    new time/op    delta
Sysbench/KV/1node_remote/oltp_point_select-10    35.2µs ±10%    35.7µs ±12%    ~     (p=0.853 n=10+10)

name                                           old alloc/op   new alloc/op   delta
Sysbench/KV/1node_remote/oltp_point_select-10    6.74kB ± 2%    6.78kB ± 1%  +0.65%  (p=0.040 n=9+9)

name                                           old allocs/op  new allocs/op  delta
Sysbench/KV/1node_remote/oltp_point_select-10      61.0 ± 0%      60.0 ± 0%  -1.64%  (p=0.000 n=10+9)
```
@nvanbenschoten
kv: allocate BatchResponse header and responses together
0165b50 
Avoids a heap allocation on each BatchResponse creations.

```
name                                           old time/op    new time/op    delta
Sysbench/KV/1node_remote/oltp_point_select-10    39.8µs ±16%    39.6µs ±19%    ~     (p=0.968 n=9+10)

name                                           old alloc/op   new alloc/op   delta
Sysbench/KV/1node_remote/oltp_point_select-10    6.79kB ± 4%    6.77kB ± 4%    ~     (p=0.671 n=10+10)

name                                           old allocs/op  new allocs/op  delta
Sysbench/KV/1node_remote/oltp_point_select-10      60.0 ± 0%      59.0 ± 0%  -1.67%  (p=0.000 n=9+8)
```
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/gRPCStreamRequestReuse branch from a3456a7 to 0165b50 Compare December 10, 2024 20:02
@cockroachdb cockroachdb deleted a comment from blathers-crl bot Dec 10, 2024
@nvanbenschoten
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 11, 2024

Build succeeded:

@craig craig bot merged commit d18eb68 into cockroachdb:master Dec 11, 2024
22 checks passed
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/gRPCStreamRequestReuse branch December 13, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaduBerinde RaduBerinde RaduBerinde approved these changes

@tbg tbg Awaiting requested review from tbg

@xinhaoz xinhaoz Awaiting requested review from xinhaoz xinhaoz was automatically assigned from cockroachdb/obs-prs

@arjunmahishi arjunmahishi Awaiting requested review from arjunmahishi arjunmahishi was automatically assigned from cockroachdb/obs-india-prs

@aa-joshi aa-joshi Awaiting requested review from aa-joshi aa-joshi was automatically assigned from cockroachdb/obs-india-prs

Assignees
No one assigned
Labels
o-perf-efficiency Related to performance efficiency
Projects
None yet
Milestone
No milestone
Development

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