resize query buffer more accurately by soloestoy · Pull Request #5013 · redis/redis

soloestoy · 2018-06-12T15:58:55Z

After commit cec404f there are still some problem we should fix I think:

c->querybuf_peak has not been updated correctly in readQueryFromClient.

void readQueryFromClient(aeEventLoop *el, int fd, void *privdata, int mask) {
    qblen = sdslen(c->querybuf);
    if (c->querybuf_peak < qblen) c->querybuf_peak = qblen;
    c->querybuf = sdsMakeRoomFor(c->querybuf, readlen);
    nread = read(fd, c->querybuf+qblen, readlen);
    ...
    sdsIncrLen(c->querybuf,nread);

As we can see, we update c->querybuf_peak before read and sdsIncrLen, it means that qblen here is the last c->querybuf length. But the length of c->querybuf after processInputBuffer is always 0, unless we didn't read the whole request at one time.

So, we should update c->querybuf_peak after sdsIncrLen I think.

We should use sdsalloc instead of sdsAllocSize.

Because we wanna get c->querybuf alloc space except for header, the sdsAllocSize value is always 32 * 1024 + sdsHdrSize + 1 grate than 32 * 1024.
~~Check if query buffer is > BIG_ARG and too big for latest peak only when c->querybuf_peak is not 0.~~
~~Function clientsCronResizeQueryBuffer reset c->querybuf_peak to be 0 no matter resize the query buffer or not.~~
~~If we don't get any requests after that, next time we will trigger the condition definitely.~~
Query buffer shrinking improvements
when tracking the peak, don't reset the peak to 0, reset it to the maximum of the current used, and the planned to be used by the current arg.
when shrining, split the two separate conditions. the idle time shrinking will remove all free space. but the peak based shrinking will keep room for the current arg.
when we resize due to a peak (rahter than idle time), don't trim all unused space, let the qbuf keep a size that's sufficient for the currently process bulklen, and the current peak.

src/server.c

sundb · 2021-06-17T02:15:56Z

@oranagra @yoav-steinberg
Due to the change in #9003, a long-standing bug was raised under valgrind.
This bug can cause the master-slave sync to take a very long time, causing the pendingquerybuf.tcl test to fail.
This problem does not only occur in master-slave sync, it is triggered when the big arg is greater than 32k.
step:

dd if=/dev/zero of=bigfile bs=1M count=32
./src/redis-cli -x hset a a < bigfile

Make room for querybuf in processMultibulkBuffer, now the alloc of querybuf will be more than 32k.

redis/src/networking.c

Line 2159 in b586d5b

c->querybuf = sdsMakeRoomForNonGreedy(c->querybuf, readlen);
If this happens to trigger the clientsCronResizeQueryBuffer, querybuf will be resized to 0.
Finally, in readQueryFromClient, we expand the querybuf non-greedily, from 0 to 32k.
Old code, make room for querybuf is greedy, so it only need 11 times to expand to 32M(16k*(2^11)), but now we need 2048(32*1024/16) times to reach it, due to the slow allocation under valgrind tha exposed the problem.

oranagra · 2021-06-17T06:57:43Z

regarding #9003, shouldn't this line be:

-        if (remaining > 0 && (size_t)remaining < readlen) readlen = remaining;
+        if (remaining > 0 && (size_t)remaining > readlen) readlen = remaining;

or even just:

-        if (remaining > 0 && (size_t)remaining < readlen) readlen = remaining;
+        readlen = remaining;

i.e. this readlen is what's triggering the (non greedy) allocation, we do want to make room for the entire big arg.

regardless, i have a new realization about the shrinking.
as i said, we have to completely separate triggers, idle, and peak.
the peak comes to shrink big query buffers that are a left over from previous commands, it should NOT shrink back a query buffer that's needed for the argument we're currently reading (the current c->bulklen).

then, for the case of an idle client that did initiate sending a big arg and then became idle, the idle time (2 seconds), can shrink the query buffer despite of c->bulklen.

i suppose that fixing either of these two problems will solve the failing pendingquerybuf.tcl test, but we wanna do both.

yoav-steinberg · 2021-06-17T07:11:11Z

the peak comes to shrink big query buffers that are a left over from previous commands, it should NOT shrink back a query buffer that's needed for the argument we're currently reading (the current c->bulklen).

then, for the case of an idle client that did initiate sending a big arg and then became idle, the idle time (2 seconds), can shrink the query buffer despite of c->bulklen.

If I follow this logic through, then I'd say that peak based shrinking should only happen in case the query buf is empty sdslen(c->querybuf) == 0. In all other cases we're in the middle of a command and should never shrink (regardless of c->bulklen). If the client is idle it'll shrink regardless of sdslen(c->querybuf).

sundb · 2021-06-17T07:23:21Z

@yoav-steinberg There is still a problem with sdslen(c->querybuf), we expand querybuf to the size of big arg in processMultibulkBuffer, at this point sdslen(c->querybuf) maybe 0, use c->bulklen would be safer.

yoav-steinberg · 2021-06-17T07:38:33Z

@yoav-steinberg There is still a problem with sdslen(c->querybuf), we expand querybuf to the size of big arg in processMultibulkBuffer, at this point sdslen(c->querybuf) maybe 0, use c->bulklen would be safer.

You're right. How about sdslen(c->querybuf) == 0 && c->bulklen == -1? This way we won't shrink the qb in case of a partial inline protocol.

sundb · 2021-06-17T08:17:36Z

@yoav-steinberg querybuf is also shrinked in case of inline protocol, because the default value of c->bulklen is -1.

yoav-steinberg

safer casting..

src/server.c

…Followup for #9003) (#9100) Due to the change in #9003, a long-standing bug was raised under `valgrind`. This bug can cause the master-slave sync to take a very long time, causing the `pendingquerybuf.tcl` test to fail. This problem does not only occur in master-slave sync, it is triggered when the big arg is greater than 32k. step: ```sh dd if=/dev/zero of=bigfile bs=1M count=32 ./src/redis-cli -x hset a a < bigfile ``` 1) Make room for querybuf in processMultibulkBuffer, now the alloc of querybuf will be more than 32k. 2) If this happens to trigger the `clientsCronResizeQueryBuffer`, querybuf will be resized to 0. 3) Finally, in readQueryFromClient, we expand the querybuf non-greedily, from 0 to 32k. Old code, make room for querybuf is greedy, so it only needs 11 times to expand to 32M(16k*(2^11)), but now we need 2048(32*1024/16) times to reach it, due to the slow allocation under valgrind that exposed the problem. The fix for the excessive shrinking of the query buf to 0, will be handled in #5013 (that other change on it's own can fix failing test too), but the fix in this PR will also fix the failing test. The fix in this PR will makes the reading in `readQueryFromClient` more aggressive when working on a big arg (so that it is in par with the same code in `processMultibulkBuffer` (i.e. the two calls to `sdsMakeRoomForNonGreedy` should both use the bulk size). In the code before this fix the one in readQueryFromClient always has `readlen = PROTO_IOBUF_LEN`

oranagra · 2021-06-20T08:24:34Z

@sundb @yoav-steinberg @soloestoy please have a look at the last change i pushed.

src/sds.c

yoav-steinberg

Regarding my comments about doing the resize only when sdslen(c->querybuf)==0, I now see I was wrong. Even if we're not idle and we have excess qbuf size due to previous command we still might be in the middle of processing the current command with a non zero qbuf and want to resize.
See below my updated remarks.

src/sds.c

src/server.c

oranagra · 2021-07-01T13:47:19Z

@soloestoy can you take a look at my abuse of your PR?

src/sds.c

yoav-steinberg

Fix bug when growing to a larger sds type.

src/sds.c

oranagra · 2021-07-04T12:21:29Z

FULL CI: https://github.com/redis/redis/actions/runs/998358871

1. querybuf_peak has not been updated correctly in readQueryFromClient. 2. qbuf shrinking uses sdsalloc instead of sdsAllocSize see more details in issue redis#4983

when tracking the peak, don't reset the peak to 0, reset it to the maximum of the current used, and the planned to be used by the current arg. when shrining, split the two separate conditions. the idle time shrinking will remove all free space. but the peak based shrinking will keep room for the current arg. when we resize due to a peak (rahter than idle time), don't trim all unused space, let the qbuf keep a size that's sufficient for the currently process bulklen, and the current peak. Co-authored-by: sundb <[email protected]> Co-authored-by: yoav-steinberg <[email protected]>

…Followup for redis#9003) (redis#9100) Due to the change in redis#9003, a long-standing bug was raised under `valgrind`. This bug can cause the master-slave sync to take a very long time, causing the `pendingquerybuf.tcl` test to fail. This problem does not only occur in master-slave sync, it is triggered when the big arg is greater than 32k. step: ```sh dd if=/dev/zero of=bigfile bs=1M count=32 ./src/redis-cli -x hset a a < bigfile ``` 1) Make room for querybuf in processMultibulkBuffer, now the alloc of querybuf will be more than 32k. 2) If this happens to trigger the `clientsCronResizeQueryBuffer`, querybuf will be resized to 0. 3) Finally, in readQueryFromClient, we expand the querybuf non-greedily, from 0 to 32k. Old code, make room for querybuf is greedy, so it only needs 11 times to expand to 32M(16k*(2^11)), but now we need 2048(32*1024/16) times to reach it, due to the slow allocation under valgrind that exposed the problem. The fix for the excessive shrinking of the query buf to 0, will be handled in redis#5013 (that other change on it's own can fix failing test too), but the fix in this PR will also fix the failing test. The fix in this PR will makes the reading in `readQueryFromClient` more aggressive when working on a big arg (so that it is in par with the same code in `processMultibulkBuffer` (i.e. the two calls to `sdsMakeRoomForNonGreedy` should both use the bulk size). In the code before this fix the one in readQueryFromClient always has `readlen = PROTO_IOBUF_LEN`

when tracking the peak, don't reset the peak to 0, reset it to the maximum of the current used, and the planned to be used by the current arg. when shrining, split the two separate conditions. the idle time shrinking will remove all free space. but the peak based shrinking will keep room for the current arg. when we resize due to a peak (rahter than idle time), don't trim all unused space, let the qbuf keep a size that's sufficient for the currently process bulklen, and the current peak. Co-authored-by: sundb <[email protected]> Co-authored-by: yoav-steinberg <[email protected]>

soloestoy mentioned this pull request Jun 12, 2018

seconds hang up report #4983

Open

sundb mentioned this pull request May 30, 2021

Fix the wrong reisze of querybuf #9003

Merged

oranagra reviewed May 30, 2021

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

yoav-steinberg suggested changes Jun 17, 2021

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

oranagra mentioned this pull request Jun 17, 2021

Make readQueryFromClient more aggressive when reading big arg again (Followup for #9003) #9100

Merged

sundb reviewed Jun 20, 2021

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

oranagra requested a review from yossigo July 1, 2021 13:10

oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jul 1, 2021

yoav-steinberg reviewed Jul 1, 2021

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

oranagra reviewed Jul 1, 2021

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

yoav-steinberg suggested changes Jul 1, 2021

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

yossigo previously approved these changes Jul 4, 2021

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

oranagra dismissed yossigo’s stale review via 66a5be5 July 4, 2021 11:31

oranagra previously approved these changes Jul 4, 2021

View reviewed changes

oranagra dismissed their stale review via dfa05f2 July 4, 2021 12:18

oranagra previously approved these changes Jul 5, 2021

View reviewed changes

resize query buffer more accurately

4ccda4c

1. querybuf_peak has not been updated correctly in readQueryFromClient. 2. qbuf shrinking uses sdsalloc instead of sdsAllocSize see more details in issue redis#4983

oranagra dismissed their stale review via 38eb358 July 5, 2021 06:06

oranagra force-pushed the resize-query-buffer branch from dfa05f2 to 38eb358 Compare July 5, 2021 06:06

oranagra previously approved these changes Jul 5, 2021

View reviewed changes

oranagra dismissed their stale review via 582e1f9 July 5, 2021 06:22

oranagra force-pushed the resize-query-buffer branch from 38eb358 to 582e1f9 Compare July 5, 2021 06:22

oranagra approved these changes Jul 5, 2021

View reviewed changes

oranagra merged commit ec582cc into redis:unstable Jul 5, 2021

oranagra added release-notes indication that this issue needs to be mentioned in the release notes and removed release-notes indication that this issue needs to be mentioned in the release notes labels Jul 5, 2021

Conversation

soloestoy commented Jun 12, 2018 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sundb commented Jun 17, 2021

Uh oh!

oranagra commented Jun 17, 2021

Uh oh!

yoav-steinberg commented Jun 17, 2021

Uh oh!

sundb commented Jun 17, 2021

Uh oh!

yoav-steinberg commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sundb commented Jun 17, 2021

Uh oh!

yoav-steinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oranagra commented Jun 20, 2021

Uh oh!

Uh oh!

yoav-steinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jul 1, 2021

Uh oh!

Uh oh!

yoav-steinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jul 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

soloestoy commented Jun 12, 2018 •

edited by oranagra

Loading

yoav-steinberg commented Jun 17, 2021 •

edited

Loading