Adjust sds types #502

poiuj · 2024-05-15T12:52:26Z

sds type should be determined based on the size of the underlying buffer, not the logical length of the sds. Currently we truncate the alloc field in case buffer is larger than we can handle. It leads to a mismatch between alloc field and the actual size of the buffer. Even considering that alloc doesn't include header size and the null terminator.

It also leads to a waste of memory with jemalloc. For example, let's consider creation of sds of length 253. According to the length, the appropriate type is SDS_TYPE_8. But we allocate 253 + sizeof(struct sdshdr8) + 1 bytes, which sums to 257 bytes. In this case jemalloc allocates buffer from the next size bucket. With current configuration on Linux it's 320 bytes. So we end up with 320 bytes buffer, while we can't address more than 255.

The same happens with other types and length close enough to the appropriate powers of 2.

The downside of the adjustment is that with allocators that do not allocate larger than requested chunks (like GNU allocator), we switch to a larger type "too early". It leads to small waste of memory. Specifically: sds of length 31 takes 35 bytes instead of 33 (2 bytes wasted) sds of length 255 takes 261 bytes instead of 259 (2 bytes wasted) sds of length 65,535 takes 65,545 bytes instead of 65,541 (4 bytes wasted) sds of length 4,294,967,295 takes 4,294,967,313 bytes instead of 4,294,967,305 (8 bytes wasted)

poiuj · 2024-05-15T12:54:49Z

This patch is subset of changes in #453. I think we can merge this independently. #453 depends on this patch and on #476.

codecov · 2024-05-15T22:06:51Z

Codecov Report

Attention: Patch coverage is 76.00000% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 70.26%. Comparing base (a0aebb6) to head (a6013d2).
Report is 13 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #502      +/-   ##
============================================
+ Coverage     70.17%   70.26%   +0.08%     
============================================
  Files           109      109              
  Lines         59904    59983      +79     
============================================
+ Hits          42039    42148     +109     
+ Misses        17865    17835      -30

Files	Coverage Δ
src/sds.c	`85.81% <76.00%> (-1.32%)`	⬇️

... and 23 files with indirect coverage changes

madolson

I think this makes sense to me. The slightly more memory usage on some systems is likely OK since I think we still prefer to use jemalloc in most cases.

src/sds.c

ranshid · 2024-05-19T11:20:40Z

@poiuj overall looks much better now. I would suggest we include some unit tests to cover different types edge cases (for example the case of old style alloc overflow). I am comfortable with the asserts, but we will probably want to trigger ci and daily before we add this.

After we finish this PR lets go and fix #453 accordingly

ranshid · 2024-05-19T15:57:22Z

@poiuj the tests looks fine.
Regarding the top comment:

The downside of the adjustment is that with allocators that do not allocate larger than requested chunks (like GNU allocator), we switch to a larger type "too early". It leads to small waste of memory. Specifically: sds of length 31 takes 35 bytes instead of 33 (2 bytes wasted) sds of length 255 takes 261 bytes instead of 259 (2 bytes wasted) sds of length 65,535 takes 65,545 bytes instead of 65,541 (4 bytes wasted) sds of length 4,294,967,295 takes 4,294,967,313 bytes instead of 4,294,967,305 (8 bytes wasted)

Is it really a waste? we already reduced the overflow in the prev implementation and AFAIK we never should have written pass the usable anyhow right?
If anything this would have solved some fragmentation problem IIUC:

lets say I will allocate 253 sds.

Before: I would allocate according to 253+(sizeof(sdshdr8)=3)+1 = 257 --> allocated size of 320 (IIRC) but the usable size is 255
After: I would allocate according to 253+(sizeof(sdshdr16)=5)+1 = 259 --> allocated size of 320 (IIRC) and the usable size is 314

So IMO this is 59 bytes gain:)
I think the main gain here is that we will be able to avoid mallocsize in realloc (and the optimization in #453)

I still want to verify there are no issues related to backward compatibility (I think RDB does not serialize sds headers, but would like to think on other potential case)

poiuj · 2024-05-19T17:44:55Z

@ranshid your example is correct. It's a 59 bytes gain if we use jemalloc. My top commented is about libc allocator that wouldn't allocate 320 bytes in this case.

ranshid · 2024-05-19T18:36:24Z

@ranshid your example is correct. It's a 59 bytes gain if we use jemalloc. My top commented is about libc allocator that wouldn't allocate 320 bytes in this case.

Oh I get it now. yes. indeed this is not too bad IMO. Please also consider avoid the malloc_size use in realloc

poiuj · 2024-05-19T18:51:55Z

@ranshid sure, I plan to get rid of call to zmalloc_size in resize as part of #453.

src/sds.c

madolson

LGTM, but would like @lipzhu and @ranshid to signoff as well. This is sensitive code and would like extra eyes on it.

lipzhu · 2024-05-22T02:03:34Z

src/sds.c

@@ -124,8 +132,14 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) {
    s = (char*)sh+hdrlen;
    fp = ((unsigned char*)s)-1;
    usable = usable-hdrlen-1;
+
+#ifdef USE_JEMALLOC
+    assert(usable <= sdsTypeMaxSize(type));


The assertion here is based on the behavior we expected/monitor from jemalloc, it depends on the internal of jemalloc, is there any guarantee from jemalloc? Maybe a potential risk.

why can't we always keep the defending part? (usable = min(usable, sdsTypeMaxSize(type))
AND also maybe we need to consider keep the assert for the usable size correctness, eg:
assert(usable >= initlen);

assert(usable <= sdsTypeMaxSize(type)); assert(initlen <= usable);

Are our two invariants. Should we be doing those asserts regardless of the allocator being used?

Yes that is a good point:
we want to make sure that in case JEMALLOC is used we will SURELY have real usable == usable, in order to satisfy #453, so I guess we do have to keep assert(usable <= sdsTypeMaxSize(type)); for JEMALLOC (for example in order to assert when JEMALLOC was compiled with different bin sizes?).
I think (as @madolson suggested) the defensive option is better at this point.

I would still change the code so that:

#ifndef USE_JEMALLOC
usable = MIN(usable, sdsTypeMaxSize(type));
#endif
assert(usable <= sdsTypeMaxSize(type) && initlen <= usable);

I think there is a better solution. We can adjust types after the allocation/reallocation. Previously I thought there are some edge cases that wouldn't work. But after reconsidering the idea, and noting again that alloc doesn't include header size and null terminator, I think it's a better way.
With this solution we don't need to take assumptions for correctness. Instead of truncating usable, we're going to adjust the header type.
I'll update the PR with this idea.

sds type should be determined based on the size of the underlying buffer, not the logical length of the sds. Currently we truncate the alloc field in case buffer is larger than we can handle. It leads to a mismatch between alloc field and the actual size of the buffer. Even considering that alloc doesn't include header size and the null terminator. It also leads to a waste of memory with jemalloc. For example, let's consider creation of sds of length 253. According to the length, the appropriate type is SDS_TYPE_8. But we allocate `253 + sizeof(struct sdshdr8) + 1` bytes, which sums to 257 bytes. In this case jemalloc allocates buffer from the next size bucket. With current configuration on Linux it's 320 bytes. So we end up with 320 bytes buffer, while we can't address more than 255. The same happens with other types and length close enough to the appropriate powers of 2. The downside of the adjustment is that with allocators that do not allocate larger than requested chunks (like GNU allocator), we switch to a larger type "too early". It leads to small waste of memory. Specifically: sds of length 31 takes 35 bytes instead of 33 (2 bytes wasted) sds of length 255 takes 261 bytes instead of 259 (2 bytes wasted) sds of length 65,535 takes 65,545 bytes instead of 65,541 (4 bytes wasted) sds of length 4,294,967,295 takes 4,294,967,313 bytes instead of 4,294,967,305 (8 bytes wasted) Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Make sdsTypeMaxSize account for header and null terminator. Use it in sdsReqType and to assert `usable` is less than max size. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

The test checks that appropriate types are selected for string length. It also checks that sdsAllocSize returns correct allocation size. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Also eliminate copying of strings - pass NULL as init. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

This is to preserve git history. To be able to use it in sdsReqType a forward declaration is added. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

After the adjustments, jemalloc always returns buffer that fit to chosen SDS type. On the other hand, it's not guaranteed with other allocators. We leave the current behavior for all allocators, but jemalloc. For jemalloc we assert instead. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

With other allocators we can't guarantee alloc field is correct. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

libc allocator can return larger buffer than requested. Unlike jemalloc the buffer sizes are not aligned with SDS sizes. That's why we may need to adjust the SDS type to a larger one to be sure alloc field can hold the size of the allocated buffer. The maximum length for type SDS_TYPE_X is 2^X - header_size(SDS_TYPE_X) - 1. The maxium value to be stored in alloc field is 2^X - 1. When allocated buffer is larger than 2^X + header_size(SDS_TYPE_X), we "move" to a larger type SDS_TYPE_Y. To be sure SDS_TYPE_Y header fits into 2^X + header_size(SDS_TYPE_X) + 1 bytes, the difference between header sizes must be smaller than header_size(SDS_TYPE_X) + 1. We ignore SDS_TYPE_5 as it doesn't have alloc field. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

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

poiuj · 2024-05-23T16:53:58Z

Updated the branch. Includes the new approach with adjusting types. Also rebased on the latest unstable. Unfortunately, missed some style changes during conflict resolution. But it's small stuff that I can fix later before the actual merge.

ranshid

maybe we can make a function to capture the repeated code?:

 /* Adjust type if usable won't fit into alloc. */
    if (type != SDS_TYPE_5 && usable > sdsTypeMaxSize(type)) {
        type = sdsReqType(usable);
        hdrlen = sdsHdrSize(type);
        usable = bufsize - hdrlen - 1;
        assert(usable <= sdsTypeMaxSize(type));
    }

ranshid · 2024-05-26T06:35:05Z

src/sds.c

-    if (type == SDS_TYPE_8) return (1 << 8) - 1;
-    if (type == SDS_TYPE_16) return (1 << 16) - 1;
+    if (type == SDS_TYPE_5)
+        return (1<<5) - 1;


I think it is better to leave the current format in order to avoid non-logical changes.

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

ranshid · 2024-05-28T18:01:24Z

LGTM but would like to hear @lipzhu

lipzhu

LGTM, just one concern.

lipzhu · 2024-05-29T01:18:06Z

src/unit/test_sds.c

+    x = sdsnewlen(NULL, 252);
+    TEST_ASSERT_MESSAGE("len 252 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_8);
+#ifdef USE_JEMALLOC
+    TEST_ASSERT_MESSAGE("len 252 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));


I found the warn message from jemalloc manual.

Any discrepancy between the requested allocation size and the size reported by malloc_usable_size() should not be depended on, since such behavior is entirely implementation-dependent.

We don't depend on discrepancy between the requested allocation size and the size reported by malloc_usable_size. Not in the test, not in the code. The test checks that sdsAllocSize returns the same value as s_malloc_size. It's always true as sdsAllocSize is based on the value we got from malloc_usable_size before.
In any case, this test checks that our assumption is correct. As we vendor jemalloc, the only case when it can change (theoretically) is when we update jemalloc.

It's always true as sdsAllocSize is based on the value we got from malloc_usable_size before.

You are right, so we can open assertion not only for jemalloc?

Oh, you're right! This ifdef isn't needed anymore. Updated the PR.

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

madolson reviewed May 16, 2024

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

src/sds.c Outdated Show resolved Hide resolved

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

poiuj force-pushed the adjust-sds-types branch from 831dfe2 to 87af02a Compare May 16, 2024 20:25

poiuj force-pushed the adjust-sds-types branch from c1c0a2a to 8f308f4 Compare May 19, 2024 15:08

madolson reviewed May 20, 2024

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

lipzhu reviewed May 20, 2024

View reviewed changes

src/sds.c Outdated Show resolved Hide resolved

lipzhu mentioned this pull request May 20, 2024

Quick fix of failure when use libc allocator in daily CI. #521

Merged

poiuj force-pushed the adjust-sds-types branch 2 times, most recently from 3f6c96f to a9b378b Compare May 21, 2024 15:32

madolson requested a review from lipzhu May 21, 2024 18:22

madolson approved these changes May 22, 2024

View reviewed changes

lipzhu reviewed May 22, 2024

View reviewed changes

poiuj added 8 commits May 23, 2024 16:07

Assert usable less than max size for a type

9ce68d4

Make sdsTypeMaxSize account for header and null terminator. Use it in sdsReqType and to assert `usable` is less than max size. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Add sds typesAndAllocSize unit test

168df2d

The test checks that appropriate types are selected for string length. It also checks that sdsAllocSize returns correct allocation size. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Add SDS_TYPE_64 case in test_typesAndAllocSize

260db1c

Also eliminate copying of strings - pass NULL as init. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Move sdsTypeMaxSize function on its previous place

1689b55

This is to preserve git history. To be able to use it in sdsReqType a forward declaration is added. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

Test alloc size with jemalloc only

07565d9

With other allocators we can't guarantee alloc field is correct. Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>

madolson self-requested a review May 23, 2024 16:44

poiuj force-pushed the adjust-sds-types branch from 40f3d5b to 06981bf Compare May 23, 2024 16:50

Fix typo in test comment

75c6eb5

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

poiuj force-pushed the adjust-sds-types branch from 06981bf to 75c6eb5 Compare May 23, 2024 16:51

ranshid reviewed May 26, 2024

View reviewed changes

poiuj added 2 commits May 28, 2024 10:43

Fix style in sdsTypeMaxSize

45bafcc

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

Extract type adjustment logic into a function

565e57c

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

lipzhu reviewed May 29, 2024

View reviewed changes

Check sdsAllocSize is correct in all allocators

82c8d44

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

poiuj force-pushed the adjust-sds-types branch from bd0aed4 to 82c8d44 Compare May 29, 2024 11:45

Format the changes

a6013d2

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

poiuj force-pushed the adjust-sds-types branch from afce304 to a6013d2 Compare May 29, 2024 16:06

poiuj requested review from lipzhu and ranshid May 31, 2024 12:49

lipzhu approved these changes Jun 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust sds types #502

Adjust sds types #502

poiuj commented May 15, 2024

poiuj commented May 15, 2024

codecov bot commented May 15, 2024 •

edited

madolson left a comment

ranshid commented May 19, 2024

ranshid commented May 19, 2024 •

edited

poiuj commented May 19, 2024

ranshid commented May 19, 2024

poiuj commented May 19, 2024

madolson left a comment

lipzhu May 22, 2024

ranshid May 22, 2024

madolson May 22, 2024

ranshid May 22, 2024

ranshid May 22, 2024

poiuj May 22, 2024

poiuj commented May 23, 2024

ranshid left a comment

ranshid May 26, 2024

ranshid commented May 28, 2024

lipzhu left a comment

lipzhu May 29, 2024

poiuj May 29, 2024

lipzhu May 29, 2024

poiuj May 29, 2024

Adjust sds types #502

Are you sure you want to change the base?

Adjust sds types #502

Conversation

poiuj commented May 15, 2024

poiuj commented May 15, 2024

codecov bot commented May 15, 2024 • edited

Codecov Report

madolson left a comment

Choose a reason for hiding this comment

ranshid commented May 19, 2024

ranshid commented May 19, 2024 • edited

poiuj commented May 19, 2024

ranshid commented May 19, 2024

poiuj commented May 19, 2024

madolson left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiuj commented May 23, 2024

ranshid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranshid commented May 28, 2024

lipzhu left a 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

codecov bot commented May 15, 2024 •

edited

ranshid commented May 19, 2024 •

edited