slave buffers were wasteful and incorrectly counted causing eviction

[oranagra]

A) slave buffers didn't count internal fragmentation and sds unused space,
this caused them to induce eviction although we didn't mean for it.

B) slave buffers were consuming about twice the memory of what they actually needed.

• this was mainly due to sdsMakeRoomFor growing to twice as much as needed each time
  but networking.c not storing more than 16k (partially fixed recently in 237a387).
• besides it wasn't able to store half of the new string into one buffer and the
  other half into the next (so the above mentioned fix helped mainly for small items).
• lastly, the sds buffers had up to 30% internal fragmentation that was wasted,
  consumed but not used.

C) inefficient performance due to starting from a small string and reallocing many times.

what i changed:

• creating dedicated buffers for reply list, counting their size with zmalloc_size
• when creating a new reply node from, preallocate it to at least 16k.
• when appending a new reply to the buffer, first fill all the unused space of the
  previous node before starting a new one.

other changes:

• expose mem_not_counted_for_evict info field for the benefit of the test suite
• add a test to make sure slave buffers are counted correctly and that they don't cause eviction

[oranagra]

what was tested:
other than the two tests added to the test suite, one making sure that slave buffers don't cause eviction, and the other tests that their effect on used_memory is counted correctly, i also tested performance and memory utilization effects.

the test was to test the client output buffers of KEYS command:

src/redis-server --save "" &
redis-cli debug populate 10000000 sddddds
redis-cli keys "*" >/dev/null # repeated 3 times
redis-cli info commandstats
redis-cli shutdown

usec_per_call showed 6% improvement in execution time.
printing the client buffers size and zmalloc_size increase at the end of the keys command showed that although both versions said the output buffer is 219mb, the old code showed an increase of 240mb in zmalloc_size (10% waste).

In redis 4.0 (before 237a387) it used to be a lot higher (333mb, 52% waste), but note that this fix works here relatively well, because the items are small, and never need splitting, and i suppose it happens to hit an allocation size that doesn't have a very high internal fragmentation overhead.

@antirez this replaces #4719
Although we wanted to do more (we discussed improving efficiency of string duplication when various places call addReply*),
i couldn't make sure we agree on the design, and i didn't want to implement a lot of code again that wasn't going to be merged.
so instead i did the minimal change that fixes this important problem, without addressing the other potential improvements for now.

[antirez]

Hello @oranagra, thank you for submitting. Review today, let's see if we can put this into 5.0. Cheers.

[antirez]

Hello @oranagra, thank you for your work, I merged the PR, I've just two questions:

1. I see that when caching the master now you set c->reply_bytes = 0. I'm not sure why this is needed because otherwise the logic seems very similar to the past one.
2. Maybe we should go for uint64_t instead of size_t? I can imagine 32 bit instances accumulating huge replies and crashing.

There are a few things that were not obvious to me immediately after reading the PR, very small things that required 5 seconds of re-reading, not a big problem, but I'm modifying a few comments in order to make sure that it is obvious to newcomers. I will appreciate if you could check this commit (ready in 5 minutes). Thanks!

[oranagra]

@antirez IIRC i added that change in replicationCacheMaster when i scanned the code for all the places that do listEmpty(c->reply); and it looked odd to me that we reset the reply buffer but don't also reset the reply_bytes.
now that i look at it, and considering that we also reset c->bufpos, i think we should also reset c->sentlen in these places.

anyway, IIRC this change wasn't done due to a bug, but more as a precaution.

the comments you modify are fine.
please note that i did change the logic of setDeferredMultiBulkLength a bit.
in the past use did an sdscat operation that could have resulted in realloc, and it didn't have the check for the memmove size. i added both checks (mainly for code simplicity, not needed a realloc).

thanks for merging... we do need to find time to do the rest of the improvements opportunities we found, but since it wasn't clear which design is best in compromise between efficiency and simplicity, and since in the current version, all strings are copied anyway, there was no reason to hold off this important fix.

[oranagra]

@antirez realized i forgot to respond to your question about uint64_t instead of size_t. IIUC since this variable is counting something that is held in memory, there is no chance that on a 32bit application we'll ever need more than 32bit (the app cannot allocate that much, even with swap enabled, or memory mapped IO). so size_t is the right type IMO.

[0xtonyxia]

Hi @oranagra @antirez , about slave buffers and eviction, should we consider including replication backlog when getting not counted memory for free memory, because i think replication backlog is kind of cache and shouldn't be considered as part of data set. Plz see a former PR #4668 about this problem.