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

refactor: hoist key mutexes to ExecuteCommands #2620

Merged
merged 31 commits into from
Nov 4, 2024

Conversation

PokIsemaine
Copy link
Contributor

@PokIsemaine PokIsemaine commented Oct 24, 2024

isseus: #2563, #2562, #2617, #2473
NOTE:

  • Whether the removed TODO annotations (3 places) are reasonable
  • MSet removed the lock parameter

@PragmaTwice PragmaTwice changed the title refactor: hoist key mutexes refactor: hoist key mutexes to ExecuteCommands Oct 24, 2024
PragmaTwice
PragmaTwice previously approved these changes Oct 24, 2024
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.

MultiLockGuard used to be forced used in some commands, but now it's been likely not to be used, should we doc it in class MultiLockGuard?

src/types/redis_zset.cc Show resolved Hide resolved
src/types/redis_zset.cc Show resolved Hide resolved
src/types/redis_string.cc Show resolved Hide resolved
@PragmaTwice
Copy link
Member

Do we need to add "exclusive" flag to flushdb and flushall? cc @git-hulk

@git-hulk
Copy link
Member

git-hulk commented Oct 25, 2024

Do we need to add "exclusive" flag to flushdb and flushall? cc @git-hulk

Yes, they should. FLUSHDB and FLUSHALL will affect all keys in the same/all namespace. For example:

Thread 1: Read a list key metadata ------------------> push an element and update metadata
Thread 2: ------------------------------> FlushDB

Then the list key will update on the old metadata.

@PokIsemaine PokIsemaine marked this pull request as draft October 31, 2024 05:22
@PokIsemaine PokIsemaine marked this pull request as ready for review October 31, 2024 13:29
src/commands/blocking_commander.h Outdated Show resolved Hide resolved
src/commands/blocking_commander.h Outdated Show resolved Hide resolved
@mapleFU
Copy link
Member

mapleFU commented Oct 31, 2024

Rest LGTM

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.

LGTM, I've check the command part and execute part, but I'm not so familiar with streaming. And I think this requires others' carefully review. So would wait other to review and merge

@mapleFU mapleFU requested a review from PragmaTwice October 31, 2024 14:55
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

These impls look good to me.

Could we perform a basic benchmark (e.g. via redis-benchmark, with a relatively high pressure) for this patch?

@PragmaTwice PragmaTwice requested a review from torwig November 1, 2024 04:12
@mapleFU
Copy link
Member

mapleFU commented Nov 1, 2024

Could we perform a basic benchmark (e.g. via redis-benchmark, with a relatively high pressure) for this patch?

It might hard to play the benchmark, since this method enlarge the critical section a bit, it might get a little bit slower but might not affect too much...

@PragmaTwice
Copy link
Member

Could we perform a basic benchmark (e.g. via redis-benchmark, with a relatively high pressure) for this patch?

It might hard to play the benchmark, since this method enlarge the critical section a bit, it might get a little bit slower but might not affect too much...

Yeah I just want to confirm that the performance impact is not too large. A slight performance downgrade is expected and fine to me.

@PokIsemaine
Copy link
Contributor Author

PokIsemaine commented Nov 2, 2024

this patch

ubuntu% redis-benchmark -p 6666 -n 100000 -c 50 -q
WARNING: Could not fetch server CONFIG
PING_INLINE: 25497.20 requests per second, p50=1.175 msec                   
PING_MBULK: 26581.61 requests per second, p50=1.135 msec                   
SET: 15659.25 requests per second, p50=2.551 msec                   
GET: 23809.53 requests per second, p50=1.239 msec                   
INCR: 13029.32 requests per second, p50=3.143 msec                   
LPUSH: 10604.45 requests per second, p50=3.879 msec                   
RPUSH: 10123.51 requests per second, p50=3.927 msec                   
LPOP: 9820.29 requests per second, p50=4.039 msec                   
RPOP: 9662.77 requests per second, p50=4.103 msec                   
SADD: 13182.18 requests per second, p50=2.903 msec                   
HSET: 9415.31 requests per second, p50=4.263 msec                   
SPOP: 24003.84 requests per second, p50=1.255 msec                   
ZADD: 11944.58 requests per second, p50=3.431 msec                   
ZPOPMIN: 23490.72 requests per second, p50=1.295 msec                   
LPUSH (needed to benchmark LRANGE): 11371.39 requests per second, p50=3.415 msec                   
LRANGE_100 (first 100 elements): 14312.29 requests per second, p50=2.007 msec                   
LRANGE_300 (first 300 elements): 8036.00 requests per second, p50=3.551 msec                   
LRANGE_500 (first 500 elements): 5914.71 requests per second, p50=4.759 msec                  
LRANGE_600 (first 600 elements): 5207.79 requests per second, p50=5.415 msec                  
MSET (10 keys): 9639.48 requests per second, p50=4.311 msec                   
XADD: 10528.53 requests per second, p50=3.751 msec

ubuntu% redis-benchmark -p 6666 -n 100000 -c 50 --threads 8 -q
WARNING: Could not fetch server CONFIG
PING_INLINE: 66269.05 requests per second, p50=0.367 msec                   
PING_MBULK: 79744.82 requests per second, p50=0.351 msec                   
SET: 14198.49 requests per second, p50=2.871 msec                   
GET: 66137.57 requests per second, p50=0.415 msec                   
INCR: 12865.05 requests per second, p50=3.151 msec                   
LPUSH: 10751.53 requests per second, p50=3.783 msec                   
RPUSH: 10487.68 requests per second, p50=3.679 msec                   
LPOP: 9487.67 requests per second, p50=4.183 msec                    
RPOP: 9496.68 requests per second, p50=4.087 msec                    
SADD: 12845.22 requests per second, p50=3.079 msec                   
HSET: 8131.40 requests per second, p50=4.847 msec                  
SPOP: 33178.50 requests per second, p50=1.223 msec                   
ZADD: 12058.36 requests per second, p50=3.375 msec                   
ZPOPMIN: 33156.50 requests per second, p50=1.223 msec                   
LPUSH (needed to benchmark LRANGE): 10205.12 requests per second, p50=3.951 msec                   
LRANGE_100 (first 100 elements): 28457.60 requests per second, p50=1.327 msec                   
LRANGE_300 (first 300 elements): 14684.29 requests per second, p50=2.871 msec                   
LRANGE_500 (first 500 elements): 9878.49 requests per second, p50=4.183 msec                    
LRANGE_600 (first 600 elements): 8396.31 requests per second, p50=4.943 msec                  
MSET (10 keys): 9254.12 requests per second, p50=4.519 msec                   
XADD: 10205.12 requests per second, p50=3.935 msec

unstable branch

ubuntu% redis-benchmark -p 6666 -n 100000 -c 50 -q
WARNING: Could not fetch server CONFIG
PING_INLINE: 25510.20 requests per second, p50=1.151 msec                   
PING_MBULK: 25713.55 requests per second, p50=1.151 msec                   
SET: 17768.30 requests per second, p50=2.207 msec                   
GET: 25150.90 requests per second, p50=1.151 msec                   
INCR: 14430.01 requests per second, p50=2.839 msec                   
LPUSH: 12640.63 requests per second, p50=3.247 msec                   
RPUSH: 12396.18 requests per second, p50=3.223 msec                   
LPOP: 10765.42 requests per second, p50=3.535 msec                   
RPOP: 10266.94 requests per second, p50=3.799 msec                   
SADD: 14102.38 requests per second, p50=2.831 msec                   
HSET: 8261.05 requests per second, p50=4.791 msec                  
SPOP: 24813.89 requests per second, p50=1.215 msec                   
ZADD: 13150.97 requests per second, p50=3.111 msec                   
ZPOPMIN: 24863.25 requests per second, p50=1.207 msec                   
LPUSH (needed to benchmark LRANGE): 11687.71 requests per second, p50=3.471 msec                   
LRANGE_100 (first 100 elements): 14455.04 requests per second, p50=2.079 msec                   
LRANGE_300 (first 300 elements): 7792.41 requests per second, p50=3.671 msec                  
LRANGE_500 (first 500 elements): 5730.66 requests per second, p50=4.943 msec                  
LRANGE_600 (first 600 elements): 5166.89 requests per second, p50=5.487 msec                  
MSET (10 keys): 9764.67 requests per second, p50=4.255 msec                   
XADD: 11546.01 requests per second, p50=3.471 msec

ubuntu% redis-benchmark -p 6666 -n 100000 -c 50 --threads 8 -q
WARNING: Could not fetch server CONFIG
PING_INLINE: 79554.50 requests per second, p50=0.375 msec                   
PING_MBULK: 79681.27 requests per second, p50=0.383 msec                   
SET: 15306.90 requests per second, p50=2.703 msec                   
GET: 66269.05 requests per second, p50=0.423 msec                   
INCR: 13267.88 requests per second, p50=3.031 msec                   
LPUSH: 11367.51 requests per second, p50=3.543 msec                   
RPUSH: 11392.12 requests per second, p50=3.511 msec                   
LPOP: 9944.31 requests per second, p50=4.031 msec                    
RPOP: 9464.32 requests per second, p50=4.191 msec                    
SADD: 12841.92 requests per second, p50=3.159 msec                   
HSET: 7505.25 requests per second, p50=5.479 msec                  
SPOP: 33200.53 requests per second, p50=1.151 msec                   
ZADD: 12056.91 requests per second, p50=3.447 msec                   
ZPOPMIN: 33156.50 requests per second, p50=1.223 msec                   
LPUSH (needed to benchmark LRANGE): 11050.95 requests per second, p50=3.775 msec                   
LRANGE_100 (first 100 elements): 23446.66 requests per second, p50=1.671 msec                   
LRANGE_300 (first 300 elements): 14156.29 requests per second, p50=3.023 msec                   
LRANGE_500 (first 500 elements): 8812.90 requests per second, p50=4.791 msec                  
LRANGE_600 (first 600 elements): 7741.14 requests per second, p50=5.583 msec                  
MSET (10 keys): 9966.12 requests per second, p50=4.159 msec                    
XADD: 11050.95 requests per second, p50=3.631 msec

@PragmaTwice
Copy link
Member

Thank you for the benchmark result!

Just took a glance and it seems the performance downgrade is about 10%.
One problem: since we didn't touch these read-only commands like GET, why the performance of these commands are also downgraded?

@PragmaTwice
Copy link
Member

Hmm I took a benchmark in my local and I think it may due to unstable measurements.

@mapleFU
Copy link
Member

mapleFU commented Nov 4, 2024

Will merge if no negative comments tomorrow

Copy link

sonarqubecloud bot commented Nov 4, 2024

@PragmaTwice PragmaTwice merged commit 1e25be6 into apache:unstable Nov 4, 2024
32 checks passed
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.

4 participants