Use sdsAllocSize instead of sdsZmallocSize

[poiuj]

sdsAllocSize returns the correct size without consulting the allocator. Which is much faster than consulting the allocator. The only exception is SDS_TYPE_5, for which it has to consult the allocator.

This PR also sets alloc field correctly for embedded string objects. It assumes that no allocator would allocate a buffer larger than 259 + sizeof(robj) for embedded string. We use embedded strings for strings up to 44 bytes. If this assumption is wrong, the whole function would require a rewrite. In general case sds type adjustment might be needed. Such logic should go to sds.c.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (unstable@5166d48). Learn more about missing BASE report. Report is 45 commits behind head on unstable.

Files	Patch %	Lines
src/object.c	81.81%	2 Missing :warning:
src/debug.c	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##             unstable     #923   +/-   ##
===========================================
  Coverage            ?   70.58%           
===========================================
  Files               ?      112           
  Lines               ?    61531           
  Branches            ?        0           
===========================================
  Hits                ?    43432           
  Misses              ?    18099           
  Partials            ?        0           

Files	Coverage Δ
src/eval.c	56.54% <100.00%> (ø)
src/functions.c	95.59% <100.00%> (ø)
src/networking.c	88.75% <100.00%> (ø)
src/sds.c	86.08% <100.00%> (ø)
src/server.c	88.57% <100.00%> (ø)
src/server.h	100.00% <ø> (ø)
src/debug.c	54.32% <0.00%> (ø)
src/object.c	78.65% <81.81%> (ø)

[uriyage]

In sdsResize:

 * The sdsAlloc size will be set to the requested size regardless of the actual
 * allocation size, this is done in order to avoid repeated calls to this
 * function when the caller detects that it has excess space. */
sds sdsResize(sds s, size_t size, int would_regrow) {

Isn't it an issue?

[poiuj]

@uriyage we need to update the comment as this is no longer correct. It was changed in #476. The invariant is that sdsAllocSize always returns the correct size.

Can you share the code where the caller detects the excess space? We need to think of a different solution for that.

[uriyage]

@uriyage we need to update the comment as this is no longer correct. It was changed in #476. The invariant is that sdsAllocSize always returns the correct size.

Can you share the code where the caller detects the excess space? We need to think of a different solution for that.

@poiuj We do it in clientsCronResizeQueryBuffer, However since we no longer check with the allocator, it's probably not an issue anymore if we attempt to resize repeatedly. can you please update the sdsResize comment.

[PingXie]

Good catch on the stale code comment.

I am not sure I understand the concern about the impact of this PR on the excessive space check logic in clientsCronResizeQueryBuffer. With PR #476, the decision between using alloc vs consulting with the allocator is an efficiency question, as opposed to correctness, correct? Did I miss anything?

[poiuj]

Updated the sdsResize comment.

sdsResize still calls je_nallocx to determine if reallocation makes sense. I'm not sure how costly is this call in context of clientsCronResizeQueryBuffer. We can discuss separately if it's something that needs to be improved.

[madolson]

Overall this seems sane to me. I'm running some extra tests since we had some issues last time on the systems that don't use jemalloc.