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

Add speedb as an alternative to rocksdb #1792

Merged
merged 9 commits into from
Oct 13, 2023
Merged

Conversation

PragmaTwice
Copy link
Member

@PragmaTwice PragmaTwice commented Oct 1, 2023

It is just for performance evaluation.

Build with -DENABLE_SPEEDB=ON to test.

@aleksraiden
Copy link
Contributor

It's a good idea! We has been a some test with @torwig about integration and speedb is an interesting feature, but in regular case (no hi-load or edge-case system) an a performance bonus is not above a 5 - 10% (in my feeling)

@aleksraiden aleksraiden requested a review from torwig October 1, 2023 18:53
@xiaobiaozhao
Copy link
Contributor

LGTM, This PR is very good.

@git-hulk
Copy link
Member

git-hulk commented Oct 7, 2023

@xiaobiaozhao Would you like to have a test?

@git-hulk git-hulk marked this pull request as draft October 7, 2023 10:06
@xiaobiaozhao
Copy link
Contributor

@xiaobiaozhao Would you like to have a test?

All right, let me make time for performance test and stability test.

@Guyme
Copy link

Guyme commented Oct 8, 2023

Happy to assist in general tuning and specific Speedb features.
Once you have a full run completed and results, please share the logs with statistics.
preferably in Speedb Discord

@git-hulk
Copy link
Member

git-hulk commented Oct 9, 2023

@PragmaTwice Perhaps we can merge this PR if CI tests are good since it won't have any side effects? As well as other rocksdb API-compatible libraries if its license is compliant with Apache License requirements.

@PragmaTwice
Copy link
Member Author

@PragmaTwice Perhaps we can merge this PR if CI tests are good since it won't have any side effects? As well as other rocksdb API-compatible libraries if its license is compliant with Apache License requirements.

Currently it is not tested in CI, but I can add it to the job matrix.

@git-hulk
Copy link
Member

@Guyme CI test failure after enabling the speedb, it looks like speedb didn't remove key values that are expected to be recycled by the compact filter. Guess there is a slight behavior difference between speedb and rocksdb.

I will dive into this issue while I'm free, can help to give us a hint if you guys have any ideas?

@xiaobiaozhao
Copy link
Contributor

xiaobiaozhao commented Oct 10, 2023

image
# insertDB
docker run -d --rm redislabs/memtier_benchmark:latest -t 4 -c 20 -s 192.168.0.252 -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__"

docker run -d --rm redislabs/memtier_benchmark:latest -t 4 -c 20 -s 192.168.0.252 -p 6666 --distinct-client-seed  --key-minimum=2000000000 --key-maximum=20000000000 --random-data  --key-prefix="" --data-size=50 -n 5000000 --pipeline=100 --command "mset  __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__"

# stressDB
# test set only
docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.252 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:0

docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.253 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:0

# test get only
docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.252 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=0:1

docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.253 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:0

# test set:get 1:1
docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.252 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:1

docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.253 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:1

# test set:get 1:10
docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.252 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:10

docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.253 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=1:10

# test set:get 10:1
docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.252 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=10:1

docker run --rm redislabs/memtier_benchmark:latest -t 2 -c 20 -s 192.168.0.253 -p 6666 --key-minimum=2000000000 --key-maximum=20000000000 --random-data --key-prefix="" --data-size=50 -n 100000 --ratio=10:1

stress.LOG
192.168.0.252 rocksdb
192.168.0.253 speed

image

@hilikspdb
Copy link

hilikspdb commented Oct 10, 2023

We are trying to recreate the issue of the compaction filter (as far as we know we did not touch this area in our OS version). @xiaobiaozhao if you may please send us the benchmark that you run. We will try to see if any of our features helps and try to create a more comprehensive benchmark (more data, more threads, more tests). Please note that without enabling those features we should have the same performance as rocksdb (only more stable)
Thanks

@git-hulk
Copy link
Member

We are trying to recreate the issue of the compaction filter (as far as we know we did not touch this area in our OS version).

Hi @hilikspdb, Thanks for your reply.

I haven't dived deep into the reason yet, the last time we had this similar issue was that the trivial move may skip the compaction filter. But it was fixed after setting the compact_opts.bottommost_level_compaction to kForceOptimized.

@speedbmike
Copy link

speedbmike commented Oct 10, 2023

@git-hulk I checked the logs and reason is indeed trivial move only despite kForceOptimized.
Speedb is currently based on rocksdb 8.1.1 and I get the same 2 unit test failures when trying to run kvrocks with rocksdb 8.1.1.

This bug was probably fixed in facebook/rocksdb#11468 (but there is still a slight chance for it to happen).
Doing Compact twice in compact_test.cc should prevent this rare bug from happening (number of levels won't change during compaction).

@git-hulk
Copy link
Member

git-hulk commented Oct 11, 2023

e logs and reason is indeed trivial move only despite kForceOptimized.
Speedb is currently based on rocksdb 8.1.1 and I get the same 2 unit test failures when trying to run kvrocks with rocksdb 8.1.1.

Hi @speedbmike, thanks for your investigation. And it makes sense to compact twice to workaround this in our test cases.

@xiaobiaozhao Put his benchmark test script inside: #1792 (comment) cc @hilikspdb

@PragmaTwice PragmaTwice changed the title [DNM] Add speedb as an alternative to rocksdb Add speedb as an alternative to rocksdb Oct 11, 2023
@PragmaTwice PragmaTwice marked this pull request as ready for review October 11, 2023 12:50
git-hulk
git-hulk previously approved these changes Oct 11, 2023
mapleFU
mapleFU previously approved these changes Oct 11, 2023
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Thanks for support and benchmarking

@PragmaTwice
Copy link
Member Author

There should be some changes to NOTICE before we merge it.

Let me address it quickly.

@PragmaTwice PragmaTwice dismissed stale reviews from mapleFU and git-hulk via cc85f1b October 11, 2023 14:39
@git-hulk
Copy link
Member

git-hulk commented Oct 12, 2023

There should be some changes to NOTICE before we merge it.

Let me address it quickly.

maybe we can mentioned that we use speedb as the rocksdb alternative in README.

@hilikspdb
Copy link

We are working on enabling the speedb features in the vrocks code. The patch will be ready today. Initial performance tests show significant improvements.

@git-hulk
Copy link
Member

We are working on enabling the speedb features in the vrocks code. The patch will be ready today. Initial performance tests show significant improvements.

That's cool. Looking forward to see the performance improvement after patching!

@PragmaTwice
Copy link
Member Author

maybe we can mentioned that we use speedb as the rocksdb alternative in README.

Done.

@git-hulk git-hulk merged commit eef51ac into apache:unstable Oct 13, 2023
28 checks passed
@hilikspdb
Copy link

hilikspdb commented Oct 13, 2023

We have seen nice improvements in the code that manually enable the speedb features. Since we are about to release a minor version that will do this in a single call we will wait with this patch and add a pull request (to both upgrade the speedb and to call the method) ... Hilik

@git-hulk
Copy link
Member

@hilikspdb Thanks for your information.

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.

9 participants