Skip to content

Move SETINFO calls to ChannelHandler so they can be written along with HELLO #172

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

adam-fowler
Copy link
Collaborator

No description provided.

- Add ignore case to ValkeyPromise for commands whose results we don't care about
- Add deque of pending commands to ConnectedState
- Push remaining pending commands to active state once we receive the hello

Signed-off-by: Adam Fowler <[email protected]>
Move version to Version.swift
Rename ValkeyPromise.ignore to forget

Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Copy link

github-actions bot commented Aug 3, 2025

✅ Pull request no significant performance differences ✅

Summary One or more benchmarks, including `ValkeyBenchmarks:Connection: Create and drop benchmark` was not found in one of the baselines.

New baseline 'pull_request' is WITHIN the 'main' baseline thresholds.

Full Benchmark Comparison

Comparing results between 'main' and 'pull_request'

Host '949ed62af095' with 4 'x86_64' processors with 15 GB memory, running:
#18~24.04.1-Ubuntu SMP Sat Jun 28 04:46:03 UTC 2025

ValkeyBenchmarks

Client: GET benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 79 79 80 83 83 83 83 6
pull_request 78 79 80 81 81 81 81 6
Δ -1 0 0 -2 -2 -2 -2 0
Improvement % 1 0 0 2 2 2 2 0

Client: GET benchmark | parallel 20 | 20 concurrent connections metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 93 96 98 100 104 106 106 25
pull_request 93 96 98 100 102 104 104 24
Δ 0 0 0 0 -2 -2 -2 -1
Improvement % 0 0 0 0 2 2 2 -1

Connection: GET benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 4 4 4 4 4 4 4 8
pull_request 4 4 4 4 4 4 4 8
Δ 0 0 0 0 0 0 0 0
Improvement % 0 0 0 0 0 0 0 0

Connection: Pipeline benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 37 37 37 37 37 37 37 5
pull_request 36 37 37 37 37 37 37 5
Δ -1 0 0 0 0 0 0 0
Improvement % 3 0 0 0 0 0 0 0

HashSlot – {user}.whatever metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 19
pull_request 0 0 0 0 0 0 0 19
Δ 0 0 0 0 0 0 0 0
Improvement % 0 0 0 0 0 0 0 0

ValkeyCommandEncoder – Command with 7 words metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 749
pull_request 0 0 0 0 0 0 0 748
Δ 0 0 0 0 0 0 0 -1
Improvement % 0 0 0 0 0 0 0 -1

ValkeyCommandEncoder – Simple GET metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 1895
pull_request 0 0 0 0 0 0 0 1904
Δ 0 0 0 0 0 0 0 9
Improvement % 0 0 0 0 0 0 0 9

ValkeyCommandEncoder – Simple MGET 15 keys metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 359
pull_request 0 0 0 0 0 0 0 362
Δ 0 0 0 0 0 0 0 3
Improvement % 0 0 0 0 0 0 0 3

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

I'm afraid this pr shows, that special casing the HELLO command in the state machine, is not the absolute worst idea, even if it initially adds a bit of code.

Focusing on this approach, I would love to see a "ValkeyConnection create and drop" benchmark for it.

Generally looks okay, though.

Comment on lines 72 to 74
if pendingHelloCommand.requestID == requestID {
return pendingHelloCommand
return [pendingHelloCommand] + pendingCommands
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the cancel code here should be unreachable. HELLO and SETINFO are triggered from within the channel handler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it's unreachable, I can add a precondition instead.

@adam-fowler
Copy link
Collaborator Author

I'm afraid this pr shows, that special casing the HELLO command in the state machine, is not the absolute worst idea, even if it initially adds a bit of code.

This PR shows that having all the commands from the initial handshake in one place is the best thing. I could have pipelined the hello, client setinfo inside the connection as well.

@adam-fowler adam-fowler requested a review from fabianfett August 7, 2025 08:15
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.

2 participants