Skip to content

Conversation

@SoulPancake
Copy link
Contributor

For #844

Implement slot-based MGET batching for cluster clients

Co-authored-by: SoulPancake <[email protected]>
@jit-ci
Copy link

jit-ci bot commented Oct 4, 2025

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

Copy link
Collaborator

@rueian rueian left a comment

Choose a reason for hiding this comment

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

Functionally, it works, but can we optimize allocations?

@SoulPancake
Copy link
Contributor Author

I optimised the slice mem allocation, is there any other potential improvement I can work on? @rueian

@rueian
Copy link
Collaborator

rueian commented Oct 10, 2025

I optimised the slice mem allocation, is there any other potential improvement I can work on? @rueian

I wonder if it is possible to avoid using MGets since it allocates a make(map[uint16]Completed, 8) though I am not sure if it is allocated on the heap or the stack after compiler optimization. Would you mind checking that? If it is allocated on the stack, then it should be fine.

@SoulPancake
Copy link
Contributor Author

SoulPancake commented Oct 20, 2025

I wonder if it is possible to avoid using MGets since it allocates a make(map[uint16]Completed, 8) though I am not sure if it is allocated on the heap or the stack after compiler optimization. Would you mind checking that? If it is allocated on the stack, then it should be fine.

@rueian
make(map[uint16]Completed, 8) in MGets/JsonMGets does escape to heap.
What do you suggest we do here

@rueian
Copy link
Collaborator

rueian commented Oct 20, 2025

What do you suggest we do here

  1. Try inline MGets/JsonMGets manually.
  2. Then replacing make(map[uint16]Completed, 8) with make(map[uint16]int, 8) where the values are the indexes of cmds.s, eliminating the use of slotCmds.

@SoulPancake
Copy link
Contributor Author

I have some trouble will my personal laptop, I will address these in a week approx, hope that is okay

@SoulPancake SoulPancake force-pushed the ab/improve-MGET-helper-cluster branch from 32627b2 to 39a9e9f Compare October 29, 2025 10:17
@SoulPancake
Copy link
Contributor Author

@rueian
I inlined the grouping logic:

  1. Added CRC16 hashing and slot calculation functions (copied from internal/cmds/slot.go) to helper.go for key-to-slot mapping. Let me know if I should move this to a common util?
  2. Replaced the intl.MGets/intl.JsonMGets calls with manual key grouping into a map[uint16][]string.
  3. Built the command slice (cmds.s) directly using the client builder (client.B().Mget().Key(group...).Build().Pin() for MGet, and similarly for JsonMGet with path).
  4. Maintained a parallel cmdKeys slice to track keys per command for response parsing.
  5. Eliminated the intermediate slotCmds map of Completed structs, reducing heap pressure while preserving functionality.

helper.go Outdated
slotGroups := make(map[uint16][]string)
for _, key := range keys {
ks := slot(key)
slotGroups[ks] = append(slotGroups[ks], key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to avoid the string slice allocation here? Maybe use a map[uint16]int instead?

@SoulPancake SoulPancake requested a review from rueian October 30, 2025 16:59
@SoulPancake
Copy link
Contributor Author

Hi @rueian Any chance to review this?


func clusterMGet(client Client, ctx context.Context, keys []string) (ret map[string]RedisMessage, err error) {
ret = make(map[string]RedisMessage, len(keys))
slots := make(map[uint16][]int, len(keys)/2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @SoulPancake, you just replaced map[uint16][]string with map[uint16][]int, which actually has not much differences. Can you try avoid this temporary slice allocation in the map?

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