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

Allow to enable the async_io option to improve the performance #1215

Merged
merged 11 commits into from
Jan 28, 2023

Conversation

xiaobiaozhao
Copy link
Contributor

Here are some comparisons between async_io and normal

big value 500B

image

small value 50B

image

monitor diff

The green line is kvrocks compiled by async io,this pr.
ts* command using seek function to scan data from db.
image

summarize

cpu mem latency qps
async 110% 100% 100% 120%
normal 100% 100% 130-400% 100%

async_io is better in seek read, higher qps, lower latency, and slightly higher cpu usage. Perfect for introducing projects

@git-hulk
Copy link
Member

git-hulk commented Jan 2, 2023

Cool, I think it'd be better if we have the option to let users determine whether to enable it or not.

@torwig
Copy link
Contributor

torwig commented Jan 2, 2023

Since this option is marked as Experimental in the rocksdb codebase, I agree to make this option configurable.
@xiaobiaozhao Thank you for the benchmarks. Nice job!

@git-hulk git-hulk changed the title perf: turn on aync_io Allow to enable the async_io option to improve the performance Jan 3, 2023
kvrocks.conf Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@xiaobiaozhao It's not right to involve the server in the storage layer. You should add the async_io flag in storage or implement like the write option https://github.com/apache/incubator-kvrocks/blob/unstable/src/storage/storage.cc#L76

kvrocks.conf Outdated Show resolved Hide resolved
src/config/config.cc Outdated Show resolved Hide resolved
src/storage/storage.h Outdated Show resolved Hide resolved
src/config/config.cc Outdated Show resolved Hide resolved
src/storage/storage.cc Show resolved Hide resolved
@aleksraiden
Copy link
Contributor

Good feature, any updates? In latest rocksdb update fixed some bugs in async_io support, so this PR is very good point to improve our performance.

@git-hulk
Copy link
Member

Good feature, any updates? In latest rocksdb update fixed some bugs in async_io support, so this PR is very good point to improve our performance.

@xiaobiaozhao has updated the PR, to see if @torwig and @PragmaTwice have comments.

@xiaobiaozhao
Copy link
Contributor Author

I've tested mget and hgetall and async_io obviously gives hgetall a big boost.
If rocksdb has better updates in the future, we can continue to follow up.

image

image

@torwig
Copy link
Contributor

torwig commented Jan 24, 2023

@xiaobiaozhao Good research.
If you update your branch, and make a final change as mentioned @git-hulk, I think we can merge this PR.

torwig
torwig previously approved these changes Jan 24, 2023
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

LGTM

kvrocks.conf Outdated Show resolved Hide resolved
kvrocks.conf Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit c41bec8 into apache:unstable Jan 28, 2023
@jishengming1

This comment was marked as off-topic.

@git-hulk

This comment was marked as off-topic.

@jishengming1

This comment was marked as off-topic.

@jishengming1

This comment was marked as off-topic.

@git-hulk

This comment was marked as off-topic.

@jishengming1

This comment was marked as off-topic.

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.

6 participants