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

stop: grow stacks for async tasks #137743

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tbg
Copy link
Member

@tbg tbg commented Dec 19, 2024

Surprisingly, this seems to have helped with alloc/op, but not with CPU time, at least in the microbenchmark.

name                                   old time/op    new time/op    delta
Sysbench/SQL/3node/oltp_read_write-24    11.6ms ± 1%    11.6ms ± 2%    ~     (p=0.201 n=14+15)

name                                   old alloc/op   new alloc/op   delta
Sysbench/SQL/3node/oltp_read_write-24    2.20MB ± 3%    2.16MB ± 1%  -1.88%  (p=0.047 n=15+12)

name                                   old allocs/op  new allocs/op  delta
Sysbench/SQL/3node/oltp_read_write-24     11.0k ± 2%     10.8k ± 0%    ~     (p=0.069 n=15+12)

End-to-end testing via the sysbench-settings roachtest (10 runs) shows a significant improvement in qps1:

  max mean median % change max % change mean % change median
old 23673.17 22775.40 22684.17      
new 23955.21 23154.03 23256.25 1.19% 1.66% 2.52%

Closes #130663.

Epic: CRDB-43584

Footnotes

  1. https://docs.google.com/spreadsheets/d/1QUBZHllhk5CtDfcsJqc5eOIqUO4jVdhoYXMWj4CRQaQ/edit?gid=0#gid=0

Copy link

blathers-crl bot commented Dec 19, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member Author

tbg commented Dec 19, 2024

Before this PR, sysbench oltp_read_write on 3x8vcpu:

image

This was running on an older SHA, so the comparison isn't scientific, but I anecdotally see around 2-3% even on recent SHAs.

With this PR:

after.pb.gz

image

It's interesting that there are still a number of callers that need to grow the stack in addition to growstack.Grow.

@tbg tbg added the o-perf-efficiency Related to performance efficiency label Dec 19, 2024
@tbg tbg marked this pull request as ready for review December 19, 2024 15:12
@tbg tbg requested a review from a team as a code owner December 19, 2024 15:12
@tbg tbg requested a review from nvanbenschoten December 19, 2024 16:25
@tbg
Copy link
Member Author

tbg commented Dec 23, 2024

I ran oltp_read_write 5x on #137927, but it's so choppy:

base: 21778 22081 20964 19356 17538
with: 20764 20481 18452 20808 21295

hoping that #137930 significantly reduces the choppiness, and will revisit in the new year.

@tbg
Copy link
Member Author

tbg commented Jan 14, 2025

10x sysbench-settings/oltp_read_write on this branch:

Details

$ rg --no-filename 'queries:' | grep -Eo '[0-9]+\.[0-9][0-9]' | sort -rn
23955.21
23590.72
23451.91
23312.14
23299.67
23212.83
23201.54
22806.27
22592.12
22117.93

Whereas 10x at the preceding commit:

Details

$ rg --no-filename 'queries:' | grep -Eo '[0-9]+\.[0-9][0-9]' | sort -rn
23673.17
23378.29
23160.06
22769.08
22685.97
22682.37
22621.54
22563.36
22350.77
21869.38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
o-perf-efficiency Related to performance efficiency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: grow stacks for better performance (~3.00% cpu or more)
3 participants