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

Support RocksDB auto-tune rate limiter for the background IO #1424

Merged
merged 12 commits into from
May 17, 2023

Conversation

wanghenshui
Copy link
Contributor

@wanghenshui wanghenshui commented May 6, 2023

fix #1417

src/storage/storage.cc Outdated Show resolved Hide resolved
@PragmaTwice
Copy link
Member

PragmaTwice commented May 6, 2023

To make the CI pass, you can run ./x.py format in your local before pushing your code.

@PragmaTwice PragmaTwice changed the title feat: rocksdb support auto tune with default config Support RocksDB auto-tune rate limiter for the background IO May 6, 2023
@wanghenshui
Copy link
Contributor Author

@xiaobiaozhao please benchmark this in compaction with heavy write scenario

@mapleFU
Copy link
Member

mapleFU commented May 6, 2023

This LGTM. Let wait some benchmarking for it

@xiaobiaozhao
Copy link
Contributor

# The maximum allowed aggregated write rate of flush and compaction (in MB/s).
# If the rate exceeds max-io-mb, io will slow down.
# 0 is no limit
# Default: 500
max-io-mb 500

I think you can reuse this configuration 'max-io-mb'. If it is 0 use auto-tune.

@wanghenshui
Copy link
Contributor Author

# The maximum allowed aggregated write rate of flush and compaction (in MB/s).
# If the rate exceeds max-io-mb, io will slow down.
# 0 is no limit
# Default: 500
max-io-mb 500

I think you can reuse this configuration 'max-io-mb'. If it is 0 use auto-tune.

auto tune donesn't means auto detect write bandwidth, write bandwidth limit is always useful

@git-hulk
Copy link
Member

@xiaobiaozhao Did you test this PR?

@xiaobiaozhao
Copy link
Contributor

@xiaobiaozhao Did you test this PR?
I will test it this weekend.

@xiaobiaozhao
Copy link
Contributor

Press data into kvrocks script

docker run -d --rm redislabs/memtier_benchmark:latest -t 4 -c 20 -s 172.27.255.244 -p 6666 --distinct-client-seed --key-minimum=1000000000 --key-maximum=10000000000 --random-data --key-prefix="" --data-size=50 -n 100000000 --pipeline=100 --command "mset key data key data key data key data key data key data key data key data key data key data"

write script

docker run --rm redislabs/memtier_benchmark:latest -t 4 -c 20 -s 172.27.255.244 -p 6666 --distinct-client-seed --key-minimum=1000000000 --key-maximum=10000000000 --key-prefix="" -n 1000 --command "set key data"

mmexport1683951102262.png

mmexport1683951105206.png

mmexport1683951131259.png

@mapleFU
Copy link
Member

mapleFU commented May 13, 2023

Thanks! Do you have idea that why the performance decline in normal p100(And does all performance optimization and decline comes from unstable test result)?

@xiaobiaozhao
Copy link
Contributor

Thanks! Do you have idea that why the performance decline in normal p100(And does all performance optimization and decline comes from unstable test result)?

I think this is bias of a test, p99 p9999 should be more convincing

@git-hulk
Copy link
Member

This option may bring better performance benefits when the background compaction is heavy, not sure if the benchmark data was under this scenario. But anyway, it's good to merge since it also brings slight performance improvement.

@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit 27e843c into apache:unstable May 17, 2023
@wanghenshui wanghenshui deleted the auto_tune branch May 17, 2023 06:30
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.

Add support of RocksDB auto-tune rate limiter for the background IO
6 participants