-
Notifications
You must be signed in to change notification settings - Fork 500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add zfree_with_size #453
base: unstable
Are you sure you want to change the base?
Add zfree_with_size #453
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
I think the zmalloc_size is really expensive to update the |
@lipzhu the update of used_memory has 2 cost aspects, the zmalloc_size and also the fact it's an atomic operation.
|
@lipzhu good point about |
src/sds.c
Outdated
@@ -39,6 +39,7 @@ | |||
#include "sds.h" | |||
#include "sdsalloc.h" | |||
#include "util.h" | |||
#include "zmalloc.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…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>
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I see it is mainly used for AOF buf size. So I guess the concern is minor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I thought more about the future uses of sdsAllocSize
src/zmalloc.c
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this sentence means exactly? isn't it that the user must take full ownership on providing the REAL allocation size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the documentation does not clearly capture all that. can you please consider refactor to provide a more detailed explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Change the doc string for `zfree_with_size` Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>
Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>
src/Makefile
Outdated
# else | ||
# OPTIMIZATION+=-flto=auto | ||
# endif | ||
# endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code? why not delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. This was added by mistake.
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.