Fix the wrong reisze of querybuf#9003
Conversation
|
@sundb AFAIR you started to investigate that to solve the |
|
@oranagra This pr solves the problem of |
|
Let me see if i get the the full story here. by default when we read a query we use: readlen = PROTO_IOBUF_LEN;
....
c->querybuf = sdsMakeRoomFor(c->querybuf, readlen);PROTO_IOBUF_LEN is 16k, and sdsMakeRoomFor being greedy doubles that. so when it was combined with this check (PROTO_MBULK_BIG_ARG is set to 32k, and using if (querybuf_size > PROTO_MBULK_BIG_ARG && |
|
@oranagra It's introduced by #7875. |
|
ohh, yeah, that's the PR i meant (took the wrong one since they had a similar title) |
oranagra
left a comment
There was a problem hiding this comment.
sorry i wasn't paying close attention so far while you were investigating failures in this test. only now i bothered to read my test and remember what it was attempting to achieve.
Co-authored-by: Oran Agra <[email protected]>
|
i take it back, this is not a regression from #7875. so even before 7875 we would have shrieked the query buff right after it was allocated. |
|
):8 It's been around for 9 years. |
Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
|
@sundb @yoav-steinberg let me know if we're good to merge this one. |
|
@oranagra Sorry, I can't be in front of the PC these days. |
d539de1 to
2099bf1
Compare
yoav-steinberg
left a comment
There was a problem hiding this comment.
Comment on using sdsavail for updating readlen.
oranagra
left a comment
There was a problem hiding this comment.
much of these recent changes aren't really doing anything (e.g. the one in scripting.c). we can consider them a cleanup, but maybe since this PR is already quite big and confusing, we wanna leave that cleanup for a separate PR?
the one in tls.c seems to reveal another bug (again i think subject for another PR).
WDYT?
9e8b5b7 to
2099bf1
Compare
oranagra
left a comment
There was a problem hiding this comment.
so are we all good to merge this now?
|
@oranagra Yes. |
Fix test failure which introduced by #9003. The following case will occur when querybuf expansion will allocate memory equal to (16*1024)k. 1) make use ```CFLAGS=-DNO_MALLOC_USABLE_SIZE```. 2) ```malloc``` will not allocate more under ```alpine```.
…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`
The initialize memory of `querybuf` is `PROTO_IOBUF_LEN(1024*16) * 2` (due to sdsMakeRoomFor being greedy), under `jemalloc`, the allocated memory will be 40k. This will most likely result in the `querybuf` being resized when call `clientsCronResizeQueryBuffer` unless the client requests it fast enough. Note that this bug existed even before redis#7875, since the condition for resizing includes the sds headers (32k+6). ## Changes 1. Use non-greedy sdsMakeRoomFor when allocating the initial query buffer (of 16k). 1. Also use non-greedy allocation when working with BIG_ARG (we won't use that extra space anyway) 2. in case we did use a greedy allocation, read as much as we can into the buffer we got (including internal frag), to reduce system calls. 3. introduce a dedicated constant for the shrinking (same value as before) 3. Add test for querybuf. 4. improve a maxmemory test by ignoring the effect of replica query buffers (can accumulate many ACKs on slow env) 5. improve a maxmemory by disabling slowlog (it will cause slight memory growth on slow env).
Fix test failure which introduced by redis#9003. The following case will occur when querybuf expansion will allocate memory equal to (16*1024)k. 1) make use ```CFLAGS=-DNO_MALLOC_USABLE_SIZE```. 2) ```malloc``` will not allocate more under ```alpine```.
…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`
The initialize memory of `querybuf` is `PROTO_IOBUF_LEN(1024*16) * 2` (due to sdsMakeRoomFor being greedy), under `jemalloc`, the allocated memory will be 40k. This will most likely result in the `querybuf` being resized when call `clientsCronResizeQueryBuffer` unless the client requests it fast enough. Note that this bug existed even before redis#7875, since the condition for resizing includes the sds headers (32k+6). 1. Use non-greedy sdsMakeRoomFor when allocating the initial query buffer (of 16k). 1. Also use non-greedy allocation when working with BIG_ARG (we won't use that extra space anyway) 2. in case we did use a greedy allocation, read as much as we can into the buffer we got (including internal frag), to reduce system calls. 3. introduce a dedicated constant for the shrinking (same value as before) 3. Add test for querybuf. 4. improve a maxmemory test by ignoring the effect of replica query buffers (can accumulate many ACKs on slow env) 5. improve a maxmemory by disabling slowlog (it will cause slight memory growth on slow env).
The initialize memory of
querybufisPROTO_IOBUF_LEN(1024*16) * 2(due to sdsMakeRoomFor being greedy), underjemalloc, the allocated memory will be 40k.This will most likely result in the
querybufbeing resized when callclientsCronResizeQueryBufferunless the client requests it fast enough.Note that this bug existed even before #7875, since the condition for resizing includes the sds headers (32k+6).
Changes