Add zfree_with_size #453

poiuj · 2024-05-07T12:56:14Z

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 · 2024-05-08T00:20:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.25%. Comparing base (4176604) to head (72e6b28).
Report is 16 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #453      +/-   ##
============================================
+ Coverage     70.07%   70.25%   +0.18%     
============================================
  Files           110      110              
  Lines         59989    60043      +54     
============================================
+ Hits          42037    42184     +147     
+ Misses        17952    17859      -93

Files	Coverage Δ
src/sds.c	`85.86% <100.00%> (-0.13%)`	⬇️
src/zmalloc.c	`84.82% <100.00%> (+0.17%)`	⬆️

... and 28 files with indirect coverage changes

lipzhu · 2024-05-08T07:15:50Z

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 · 2024-05-08T11:43:06Z

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

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.
[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 · 2024-05-08T16:06:51Z

@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 · 2024-05-15T19:26:33Z

src/sds.c

@@ -39,6 +39,7 @@
 #include "sds.h"
 #include "sdsalloc.h"
 #include "util.h"
+#include "zmalloc.h"


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.

src/sds.c

…to align the logic with sdsnewlen function. (#476) This patch try to correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. Maybe the #453 optimization should depend on this. Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>

…to align the logic with sdsnewlen function. (valkey-io#476) This patch try to correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. Maybe the valkey-io#453 optimization should depend on this. Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>

…to align the logic with sdsnewlen function. (valkey-io#476) This patch try to correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. Maybe the valkey-io#453 optimization should depend on this. Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com> Signed-off-by: Samuel Adetunji <adetunjithomas1@outlook.com>

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. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

sdsalloc returns sds's length for SDS_TYPE_5. It's not correct for SDS_TYPE_5. This patch makes sdsAllocSize call zmalloc_size for SDS_TYPE_5. sdsalloc is a lower level function that continues to return length for SDS_TYPE_5. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Instead of zmalloc_size use s_malloc_size. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

ranshid · 2024-06-11T05:56:42Z

src/sds.c

+    char type = s[-1] & SDS_TYPE_MASK;
+    /* SDS_TYPE_5 header doesn't contain the size of the allocation */
+    if (type == SDS_TYPE_5) {
+        return s_malloc_size(sdsAllocPtr(s));


I guess this is currently my only concern with this PR. IIUC before this PR this would have been a very slim and trivial operation to call sdsAllocSize for SDS_TYPE_5, but now this will become more expensive. I guess in your tests this is being compensated by the fact that we eliminated it in free, but in some cases (like evictions) we might experience overhead?
Can we try and test CPU when heavy evictions are in place?

Actually I see it is mainly used for AOF buf size. So I guess the concern is minor

I guess my main point here is that we do not optimize SDS_TYPE_5 in this pr but we do impact the way we calculate it's allocation size. This makes sense from cleaner code maybe, but I wonder if changing it is something we want.

anyway, since it DOES make the sdsResize and sdsFree code much cleaner I am in favor of approving this PR. (maybe @madolson will think differently?)

Also note that SDS_TYPE_5 will not be used for AOF. AOF buffer is initialized with an empty SDS to be expanded later. In this case we use SDS_TYPE_8.

I agree. I thought more about the future uses of sdsAllocSize

src/zmalloc.c

ranshid · 2024-06-11T07:01:38Z

src/zmalloc.c

-    size_t oldsize;
+/* Frees the memory buffer pointed by ptr and updates statistics.
+ * ptr must already point to the start of the buffer. On systems where we store
+ * an additional header, the caller must do the necessary adjustments.


what does this sentence means exactly? isn't it that the user must take full ownership on providing the REAL allocation size?

If we don't have malloc_size on the system, we keep an extra header with the buffer size. In this case, the caller should adjust the pointer, to point to the start of the buffer. Not only to the the start of the logical object.
And, of course, the caller is responsible to provide the real allocation size.
If the caller can't satisfy any of the 2 conditions above, it should use zfree instead.

I think the documentation does not clearly capture all that. can you please consider refactor to provide a more detailed explanation?

src/zmalloc.c

Change the doc string for `zfree_with_size` Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

eliblight · 2024-06-11T15:11:58Z

src/Makefile

+# 	else
+# 		OPTIMIZATION+=-flto=auto
+# 	endif
+# endif


Dead code? why not delete?

Oops. This was added by mistake.

eliblight · 2024-06-11T15:31:21Z

src/sds.c

@@ -195,7 +195,7 @@ sds sdsdup(const sds s) {
 /* Free an sds string. No operation is performed if 's' is NULL. */
 void sdsfree(sds s) {
    if (s == NULL) return;
-    s_free((char *)s - sdsHdrSize(s[-1]));
+    s_free_with_size(sdsAllocPtr(s), sdsAllocSize(s));


I feel this is ugly AND I feel this is not your fault.

The mix between requested length and allocated size is non intuitive and error prone.

I suggest:
1/ sdsalloc() will explicitly refuse to work for SDS_TYPE_5 by asserting
case SDS_TYPE_5: assert(0, "Type 5 does not have alloc")
which is fair as Type 5 does not have alloc value and any answer is wrong

2/ similarly sdsAllocSize function is simply and will fail spectacularly on Type 5

size_t sdsAllocSize(sds s) { return sdsHdrSize(s[-1] & SDS_TYPE_MASK) + sdsalloc(s) + 1; } // how come we dont have sdsTYPE(s) macro? // Why sdsHdrSize does not take SDS but take a type? // Why dont we have sdsHdrSizeByType // So many questions...

And thus, should never be called for TYPE_5

3/ If its interesting anywhere to anyone... add a function which explicitly fuse the meaning and with a very descriptive comment as to what it actually returns

size_t sdsAllocSizeOrLen(sds s) { if (s[-1] & SDS_TYPE_MASK == SDS_TYPE_5) { return sdsLen(s) + 1; } return sdsAllocSize(s); }

I suspect this function is not really needed at all

4/ And just here in sdsFree do the if else...

void sdsfree(sds s) { if (s == NULL) return; // By the way this is lazy we should not allow sdsfree-ing a null pointer... but thats a point for future PR as there is probably code that relay on this thing if (flags & SDS_TYPE_MASK == SDS_TYPE_5) { s_free((char *)s - 1); } else { s_free_with_size(sdsAllocPtr(s), sdsAllocSize(s[-1])); }

Just peeked and following my suggestion will actually unnecessarily bloat this PR.

So I am OK with the PR as is... and its worth considering a future cleanup in a separate PR

Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

lipzhu mentioned this pull request May 8, 2024

Significance of absolute accurate used_memory? #467

Open

poiuj force-pushed the unstable branch from 35924f9 to 883f63d Compare May 8, 2024 11:13

lipzhu mentioned this pull request May 9, 2024

Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. #476

Merged

poiuj mentioned this pull request May 15, 2024

Adjust sds types #502

Merged

madolson reviewed May 15, 2024

View reviewed changes

madolson reviewed May 16, 2024

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

madolson requested a review from ranshid May 16, 2024 16:53

poiuj added 3 commits June 3, 2024 15:14

Do not include zmalloc into sds.c

37ad930

Instead of zmalloc_size use s_malloc_size. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

poiuj force-pushed the unstable branch from 779ba8c to 37ad930 Compare June 3, 2024 15:37

poiuj requested a review from madolson June 3, 2024 16:15

ranshid reviewed Jun 11, 2024

View reviewed changes

src/zmalloc.c Outdated Show resolved Hide resolved

ranshid reviewed Jun 11, 2024

View reviewed changes

src/zmalloc.c Outdated Show resolved Hide resolved

poiuj requested a review from ranshid June 11, 2024 08:46

Apply suggestions from code review

21acd10

Change the doc string for `zfree_with_size` Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

poiuj force-pushed the unstable branch from 1639a2b to 21acd10 Compare June 11, 2024 08:49

Update zfree_with_size() docstring

0c2048d

Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

eliblight reviewed Jun 11, 2024

View reviewed changes

Revert Makefile changes pushed by mistake

72e6b28

Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zfree_with_size #453

Add zfree_with_size #453

poiuj commented May 7, 2024 •

edited

codecov bot commented May 8, 2024 •

edited

lipzhu commented May 8, 2024 •

edited

zvi-code commented May 8, 2024 •

edited

poiuj commented May 8, 2024

madolson May 15, 2024 •

edited

ranshid Jun 11, 2024 •

edited

ranshid Jun 11, 2024

ranshid Jun 11, 2024

ranshid Jun 11, 2024

poiuj Jun 11, 2024 •

edited

ranshid Jun 11, 2024

ranshid Jun 11, 2024

poiuj Jun 11, 2024

ranshid Jun 11, 2024 •

edited

poiuj Jun 11, 2024

eliblight Jun 11, 2024

poiuj Jun 11, 2024

eliblight Jun 11, 2024

eliblight Jun 11, 2024

Add zfree_with_size #453

Are you sure you want to change the base?

Add zfree_with_size #453

Conversation

poiuj commented May 7, 2024 • edited

Description

Benchmark

codecov bot commented May 8, 2024 • edited

Codecov Report

lipzhu commented May 8, 2024 • edited

zvi-code commented May 8, 2024 • edited

poiuj commented May 8, 2024

madolson May 15, 2024 • edited

Choose a reason for hiding this comment

ranshid Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiuj Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranshid Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiuj commented May 7, 2024 •

edited

codecov bot commented May 8, 2024 •

edited

lipzhu commented May 8, 2024 •

edited

zvi-code commented May 8, 2024 •

edited

madolson May 15, 2024 •

edited

ranshid Jun 11, 2024 •

edited

poiuj Jun 11, 2024 •

edited

ranshid Jun 11, 2024 •

edited