Add zfree_with_size

[poiuj]

Description

zfree updates memory statistics. It gets the size of the buffer from jemalloc by calling zmalloc_size. This operation is costly. We can avoid it if we know the buffer size. For example, we can calculate size of sds from the data we have in its header.

This commit introduces zfree_with_size function that accepts both pointer to a buffer, and its size. zfree is refactored to call zfree_with_size.

sdsfree uses the new interface for all but SDS_TYPE_5.

Benchmark

Dataset is 3 million strings. Each benchmark run uses its own value size (8192, 512, and 120). The benchmark is 100% write load for 5 minutes.

value size       new tps      old tps      %       new us/call    old us/call    %
8k               272088.53    269971.75    0.78    1.83           1.92           -4.69
512              356881.91    352856.72    1.14    1.27           1.35           -5.93
120              377523.81    368774.78    2.37    1.14           1.19           -4.20

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.94%. Comparing base (ba9dd7b) to head (883f63d).
Report is 10 commits behind head on unstable.

❗ Current head 883f63d differs from pull request most recent head 779ba8c. Consider uploading reports for the commit 779ba8c to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #453      +/-   ##
============================================
- Coverage     68.95%   68.94%   -0.02%     
============================================
  Files           109      109              
  Lines         61793    61798       +5     
============================================
- Hits          42611    42604       -7     
- Misses        19182    19194      +12     

Files	Coverage Δ
src/sds.c	86.96% <100.00%> (+0.04%)	⬆️
src/zmalloc.c	83.65% <100.00%> (+0.18%)	⬆️

... and 9 files with indirect coverage changes

[lipzhu]

I think the zmalloc_size is really expensive to update the used_memory, we had a similar discussion in #308 (comment), for this patch, I'm curious if the assertion of sds->alloc == allocate size in all cases, what happened if call the sdsResize ?

[zvi-code]

@lipzhu the update of used_memory has 2 cost aspects, the zmalloc_size and also the fact it's an atomic operation.

1. regarding zmalloc_size cost: it is actually called twice, once in zmalloc code and enother one inside the je_free because the tcache needs to know what bin the buffer belongs to. So, if you remove the zmalloc_size call for stat, you are still paying the cost. When using je_sdallocx to free (this PR) it uses, internally in jemalloc, fast_path that avoids fetching the size to begin with. BTW: I think for cpp delete operator is actually overloaded in jemalloc.cpp to use sized delete.
2. [edit I see you already have PR for this] regarding atomic cost: this I believe can be also eliminated with use of thread local vars(I guess the team has considered this before)

[poiuj]

@lipzhu good point about sds->alloc == allocate size. I was sure it is. But now I see that sds->alloc is truncated when the sds len is close enough to sds type max len (2^8, 2^16, etc.). I'll update the PR with a fix.

[madolson]

We don't really want zmalloc in sds, since it uses sdsalloc.

EDIT: I see now we call zmalloc in this file anyways, and probably should have been imported before, but we would still like not have a dependency on zmalloc in this file.

[madolson]

zmalloc_size is not on all platforms! You can see the previous usage of zmalloc_size was inside a jemalloc ifdef

[poiuj]

If zmalloc_size from jemalloc is unavailable, we use our own implementation – 

valkey/src/zmalloc.c

Line 356 in 7a9951f

size_t zmalloc_size(void *ptr) {

[madolson]

I guess I initially thought wasn't ideal because it wasn't accurate, but if we don't have alloc_size on the system we are going to call free anyways.