Skip to content

Conversation

@ebarskiy
Copy link

COMMAND command is expensive to process (too much string operations)
but result is rarely changes, it makes sense to add caching

Signed-off-by: Evgeny Barskiy <[email protected]>
Signed-off-by: Evgeny Barskiy <[email protected]>
Copy link
Member

@JimB123 JimB123 left a comment

Choose a reason for hiding this comment

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

What evidence is there to suggest that COMMAND

is expensive to process (too much string operations)

How meaningful is the benefit of caching this information? I can't tell if this represents premature optimization (adding complexity without sufficient value).

}

/* Collect all output from a caching client (both buffer and reply list) */
static sds collectCachedResponse(client *c) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a common function associated with a cached response client. It's unclear why this is different from aggregateClientOutputBuffer and I tagged @roshkhatri for info.

Copy link
Author

Choose a reason for hiding this comment

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

aggregateClientOutputBuffer assumes c->bufpos == 0 which is not the case here as Add* functions utilize c->buf

Copy link
Member

Choose a reason for hiding this comment

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

FYI - I talked with @roshkhatri. He agrees that the existing function aggregateClientOutputBuffer should be updated rather than creating a new function. His existing function assumed c->bufpos == 0 for his specific use case, but this does not have to be generally true. We can update that function.

Comment on lines +5253 to +5254
/* Forward declaration */
void addReplyCommandInfo(client *c, struct serverCommand *cmd);
Copy link
Member

Choose a reason for hiding this comment

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

Just move cacheCommandInfo after addReplyCommandInfo, avoiding the need for a forward declaration.

Copy link
Author

Choose a reason for hiding this comment

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

there is a recursion to process subcommands

Signed-off-by: Evgeny Barskiy <[email protected]>
@ebarskiy
Copy link
Author

What evidence is there to suggest that COMMAND

is expensive to process (too much string operations)

How meaningful is the benefit of caching this information? I can't tell if this represents premature optimization (adding complexity without sufficient value).

@PingXie can you share internal latency of COMMAND?

Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

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

Have a similar question in the lines of @JimB123 requested.

Why do we want to optimize this particular command? This is not a heavily used command AFAIK. Apart from the clients invoking it upfront to build certain metadata. Does this lead to tail latency on your end for datapath commands?

Comment on lines +2631 to +2632
sds info_cache_resp2; /* Cached COMMAND INFO response for RESP2 */
sds info_cache_resp3; /* Cached COMMAND INFO response for RESP3 */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would recommend building an array similar to cached_cluster_slot_info.

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.

3 participants