memory reporting of clients argv by oranagra · Pull Request #7874 · redis/redis

oranagra · 2020-10-01T07:55:06Z

track and report memory used by clients argv.
this is very usaful in case clients started sending a command and didn't
complete it. in which case the first args of the command are already
trimmed from the query buffer.

in an effort to avoid cache misses and overheads while keeping track of
these, i avoid calling sdsZmallocSize and instead use the sdslen /
bulk-len which can at least give some insight into the problem.

This memory is now added to the total clients memory usage, as well as
the client list.

track and report memory used by clients argv. this is very usaful in case clients started sending a command and didn't complete it. in which case the first args of the command are already trimmed from the query buffer. in an effort to avoid cache misses and overheads while keeping track of these, i avoid calling sdsZmallocSize and instead use the sdslen / bulk-len which can at least give some insight into the problem. This memory is now added to the total clients memory usage, as well as the client list.

oranagra · 2020-10-01T07:56:13Z

This is a portion of #5159 (applying Salvatore's feedback of using the bulk length or sdslen instead of sdsZmallocSize (not tracking internal fragmentation and unused sds memory, in order to reduce the risk of any overhead on performance)

oranagra · 2020-10-01T08:31:34Z

I can add a test that tests it (sends an incomplete command) if you feel that's useful.

yossigo · 2020-10-04T16:51:49Z

src/networking.c

+    /* Compute the total memory consumed by this client. */
+    size_t obufmem = getClientOutputBufferMemoryUsage(client);
+    size_t total_mem = obufmem;
+    total_mem += sizeof(client); /* includes client->buf */


nit: we could also get actual allocation of client rather than sizeof. Probably totally negligible.

it might not be negligible, it's likely to have some 30% overhead.

30% of just sizeof(client) is not that much, but yeah it could be noticeable in some scenarios.

note that i fixed that in both CLIENT LIST, and the cron that collects data for INFO MEMORY

yossigo · 2020-10-04T16:55:40Z

src/server.h

    size_t querybuf_peak;   /* Recent (100ms or more) peak of querybuf size. */
    int argc;               /* Num of arguments of current command. */
    robj **argv;            /* Arguments of current command. */
+    size_t argv_sum;        /* Sum of lengths of objects in argv list. */


I'd consider a more descriptive name, e.g. argv_mem.

i wanted to denote that it's the sum of lengths (chars) and not memory (includes internal frag and sds unused space).
maybe you have a better suggestion?

used argv_len_sum

In some cases one command added a very big bulk of memory, and this would be "resolved" by the eviction before the next command. Seeing an unexplained mass eviction we would wish to know the highest momentary usage too. Tracking it in call() and beforeSleep() adds some hooks in AOF and RDB loading. The fix in clientsCronTrackExpansiveClients is related to #7874

track and report memory used by clients argv. this is very usaful in case clients started sending a command and didn't complete it. in which case the first args of the command are already trimmed from the query buffer. in an effort to avoid cache misses and overheads while keeping track of these, i avoid calling sdsZmallocSize and instead use the sdslen / bulk-len which can at least give some insight into the problem. This memory is now added to the total clients memory usage, as well as the client list. (cherry picked from commit bea40e6)

In some cases one command added a very big bulk of memory, and this would be "resolved" by the eviction before the next command. Seeing an unexplained mass eviction we would wish to know the highest momentary usage too. Tracking it in call() and beforeSleep() adds some hooks in AOF and RDB loading. The fix in clientsCronTrackExpansiveClients is related to #7874 (cherry picked from commit 457b707)

track and report memory used by clients argv. this is very usaful in case clients started sending a command and didn't complete it. in which case the first args of the command are already trimmed from the query buffer. in an effort to avoid cache misses and overheads while keeping track of these, i avoid calling sdsZmallocSize and instead use the sdslen / bulk-len which can at least give some insight into the problem. This memory is now added to the total clients memory usage, as well as the client list.

In some cases one command added a very big bulk of memory, and this would be "resolved" by the eviction before the next command. Seeing an unexplained mass eviction we would wish to know the highest momentary usage too. Tracking it in call() and beforeSleep() adds some hooks in AOF and RDB loading. The fix in clientsCronTrackExpansiveClients is related to redis#7874

track and report memory used by clients argv. this is very usaful in case clients started sending a command and didn't complete it. in which case the first args of the command are already trimmed from the query buffer. in an effort to avoid cache misses and overheads while keeping track of these, i avoid calling sdsZmallocSize and instead use the sdslen / bulk-len which can at least give some insight into the problem. This memory is now added to the total clients memory usage, as well as the client list. (cherry picked from commit bea40e6)

In some cases one command added a very big bulk of memory, and this would be "resolved" by the eviction before the next command. Seeing an unexplained mass eviction we would wish to know the highest momentary usage too. Tracking it in call() and beforeSleep() adds some hooks in AOF and RDB loading. The fix in clientsCronTrackExpansiveClients is related to redis#7874 (cherry picked from commit 457b707)

oranagra requested review from soloestoy and yossigo October 1, 2020 07:55

yossigo reviewed Oct 4, 2020

View reviewed changes

CR changes

9819fa0

oranagra requested a review from yossigo October 5, 2020 06:00

CR changes

9c79c50

yossigo previously approved these changes Oct 5, 2020

View reviewed changes

Merge branch 'unstable' into track_client_argv

e1e4393

oranagra requested a review from yossigo October 5, 2020 07:39

oranagra dismissed yossigo’s stale review via e1e4393 October 5, 2020 07:39

yossigo approved these changes Oct 5, 2020

View reviewed changes

oranagra mentioned this pull request Oct 5, 2020

update client list fields: tracking flags and argv-mem / tot-mem redis/redis-doc#1409

Merged

oranagra merged commit bea40e6 into redis:unstable Oct 5, 2020

oranagra deleted the track_client_argv branch October 5, 2020 08:15

oranagra mentioned this pull request Oct 8, 2020

INFO report peak memory before eviction #7894

Merged

oranagra mentioned this pull request Oct 26, 2020

release 6.0.9 #7965

Merged

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 10, 2020

oranagra mentioned this pull request Jan 13, 2021

Redis 6.2 RC1. #8187

Merged

oranagra mentioned this pull request Mar 24, 2021

Add Query Buffer used memory metric into INFO #4574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memory reporting of clients argv#7874

memory reporting of clients argv#7874
oranagra merged 4 commits intoredis:unstablefrom
oranagra:track_client_argv

oranagra commented Oct 1, 2020

Uh oh!

oranagra commented Oct 1, 2020

Uh oh!

oranagra commented Oct 1, 2020

Uh oh!

yossigo Oct 4, 2020

Uh oh!

oranagra Oct 4, 2020

Uh oh!

yossigo Oct 4, 2020

Uh oh!

oranagra Oct 5, 2020

Uh oh!

yossigo Oct 4, 2020

Uh oh!

oranagra Oct 4, 2020

Uh oh!

yossigo Oct 4, 2020

Uh oh!

oranagra Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

oranagra commented Oct 1, 2020

Uh oh!

oranagra commented Oct 1, 2020

Uh oh!

oranagra commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants