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

Optimized initialization of Redis::Cluster #912

Merged
merged 1 commit into from Jun 5, 2020

Conversation

zanker-stripe
Copy link
Contributor

@zanker-stripe zanker-stripe commented Jun 4, 2020

We were noticing that initializing a Redis Cluster client was fairly slow, and after a coworker dug into it, it turned out to come primarily from build_slot_node_key_map. Since the current implementation does heavy amount of allocation/work, when you consider that each node in a cluster has the slots iterated over, which leads to about 16,384 iterations * nodes.

Using the redis-rb cluster configuration as an example, it takes about 17ms to initialize and 49k objects, we saw significantly higher in production, but we're also running a lot more nodes. The PR gets it down to about ~1.5ms and 9 allocations.

The optimization works by building up the primary/secondary node configuration off of slots_arr instead. My understanding, is secondaries are always mirroring the slot configuration of a primary, so this trick is fine.

After we have that, we simply load up the slot configuration by adding our existing hash, which ensures we don't have to allocate a new object for every single slot. When we call put (from MOVED), we duplicate the map to ensure only that slot is changed.

I dropped the use of Set, since Ruby sets are inefficient as they're just wrappers around hashes, and the number of secondaries is sufficiently low enough that checking include? is not the end of the world.

It also means that you no longer have to call to_a each time you call find_node_key_of_slave. If you somehow had a configuration where you had tens of thousands of secondaries, your find_node_key_of_slave call would be slow anyway, and now only your initialization is slow.

Let me know if I'm misunderstanding about the Redis Cluster setup that makes this not work though.

My understanding is the existing Redis tests should cover this, and so it doesn't need additional tests, but happy to add something if you want.

Copy link
Collaborator

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@supercaracal would you like to give this a second look?

Copy link
Contributor

@supercaracal supercaracal left a comment

Choose a reason for hiding this comment

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

I apologize for my low-quality codes. Thank you for your optimization. LGTM

@byroot byroot merged commit 92a983a into redis:master Jun 5, 2020
@zanker-stripe
Copy link
Contributor Author

The code worked and got the job done and lasted a while, nothing wrong with that!

@ob-stripe
Copy link

@byroot Hi! Thanks for the quick merge. Any chance you could release a new version soon?

@byroot
Copy link
Collaborator

byroot commented Jun 8, 2020

There is a couple PRs I'd like to include first, I need to find some time to work on it. I'll try to get it out in a couple days max.

Can't you point your Gemfile to the git repo in the meantime?

@ob-stripe
Copy link

We can wait a couple of days :) Thanks!

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.

None yet

4 participants