[valkey-benchmark] Allow multiple random (or sequential) placeholders and fix zadd --sequential to create expected data

[rainsupreme]

This extends the __rand_int__ pattern to add __rand_1st__ through __rand_9th__.

For the new placeholders, multiple occurrences within a command will have the same value.

With the --sequential option, each placeholder will use separate counters.

For __rand_int__, the behavior is unchanged - multiple occurrences of the pattern within a command will have different values.

Examples of using the __rand_int__ with --sequential:

$ ./valkey-benchmark -r 300 -n 300 --sequential -q -- set key__rand_int__ val__rand_int__
set key__rand_int__ val__rand_int__: 60000.00 requests per second, p50=0.391 msec
$ ./valkey-cli info keyspace
# Keyspace
db0:keys=150,expires=0,avg_ttl=0
$ ./valkey-cli get key000000000050
"val000000000051"

For the new patterns, multiple occurrences within the command will have the same value.

$ ./valkey-benchmark -r 300 -n 300 --sequential -q -- set key__rand_1st__ val__rand_1st__
set key__rand_int__ val__rand_int1_: 60000.00 requests per second, p50=0.383 msec
$ ./valkey-cli info keyspace
# Keyspace
db0:keys=300,expires=0,avg_ttl=0
$ ./valkey-cli get key000000000050
"val000000000050"

I also fixed the zadd benchmark so it produces the expected number of keys when used with --sequential. (By using the same counter twice per command it was effectively counting by twos)

I made this for myself but raised a PR in case y'all like it. 🙂

[zuiderkwast]

Nice!

Have you considered __rand_1st__, __rand_2nd__, __rand_3rd__, __rand_4th__...? Looks nicer?

I want this for random values too!

It's in my mental backlog since I did #2057. I haveaslightly different behavior in mind though. In a transaction or pipeline sequence, I want use the same random number multiple times, for example:

MULTI ; GET k__rand_1st__ ; PEXPIRETIME k__rand_1st__ ; EXEC

(This is a simulation of the suggested GETPXT command in #1455.)

Same placeholder, same value.

Would this semantic work for sequential too?

[rainsupreme]

Hm, interesting. So you're suggesting that it only retrieve one value per placeholder in a command, whether that's random or sequential? I like that - with multiple placeholders you aren't stuck with duplicating the same value either.

How to implement it is not immediately obvious though. The way benchmark is written, for pipelined commands it just duplicates the test command multiple times into a string and forgets about it. It doesn't currently track which parts of the string correspond to prefix/auth stuff or which copy of the command. I'm pretty sure each client sends X pipelined commands and then waits for X replies before sending another batch of commands. (Assuming clients are kept alive)

[zuiderkwast]

How to implement it is not immediately obvious though. The way benchmark is written, for pipelined commands it just duplicates the test command multiple times into a string and forgets about it.

I have an idea: We can do one replacement when we write the pipeline to the buffer and another replacement when we execute it.

The first replacement inserts some special marker each time a placeholder's value (or counter) should be bumped, alternately when the placeholder's value should not be bumped.

Example command sequence: HSET __rand_1st__ __rand_2nd__ __rand_3rd__ ; EXPIRE __rand_1st__ 10.

With -P 3 we repeat this sequence 3 times into the buffer as RESP, and we modify the placeholders with some marker # for when the placeholder's counter shouldn't be bumped, i.e. all occurrences except the first of each placeholder. The buffer now looks like below (but in RESP):

HSET __rand_1st__ __rand_2nd__ __rand_3rd__ ; EXPIRE __rand#1st__ 10
HSET __rand_1st__ __rand_2nd__ __rand_3rd__ ; EXPIRE __rand#1st__ 10
HSET __rand_1st__ __rand_2nd__ __rand_3rd__ ; EXPIRE __rand#1st__ 10

When we execute it, we replace each placeholder with a random or sequential value and we don't bump it at the # placeholder.

Sounds doable?

[rainsupreme]

Ok, I came up with a revision that is used like you suggested! If the same replacement is used with the same command string it will have the same value, and with --sequential they each use independent counters as before. I also made the placeholder location data global rather than duplicated across all clients. It's initialized before threads are created, and is not modified during a test.

I also revised the placeholder strings, though I'm reluctant to change the original __rand_int__ to avoid breaking existing scripts/blogs/documentation/etc.

[codecov]

Codecov Report

Attention: Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.31%. Comparing base (e53e048) to head (4e25d21).
Report is 5 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/valkey-benchmark.c	98.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2102      +/-   ##
============================================
- Coverage     71.43%   71.31%   -0.12%     
============================================
  Files           123      123              
  Lines         67030    67060      +30     
============================================
- Hits          47881    47827      -54     
- Misses        19149    19233      +84     

Files with missing lines	Coverage Δ
src/valkey-benchmark.c	61.28% <98.66%> (+0.92%)	⬆️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zuiderkwast]

Ok, I came up with a revision that is used like you suggested! If the same replacement is used with the same command string it will have the same value, and with --sequential they each use independent counters as before. I also made the placeholder location data global rather than duplicated across all clients. It's initialized before threads are created, and is not modified during a test.

Awesome!

I also revised the placeholder strings, though I'm reluctant to change the original __rand_int__ to avoid breaking existing scripts/blogs/documentation/etc.

Yeah, we should probably not do this for __rand_int__. We can increment/bump that one at every occurrence to keep backward compatibility. We can use the new behavior only for the new placeholders. We'd just need to explain it in the --help text and then it will not be too confusing for users, or WDYT?

[rainsupreme]

@zuiderkwast I've implemented your suggestions. What do you think? 😁

[zuiderkwast]

Great!

[zuiderkwast]

Great! Just a very minor nit. Clang-format and spellchecker have some comments.

[zuiderkwast]

Need to add "nd" (for 2nd) to the wordlist I guess, .config/typos.toml. There is already an "nd" under [type.c.extend-identifiers] but we can move it to [default.extend-words] maybe.

[zuiderkwast]

Can you update the title and top comment of the PR to what was finally implemented?