chore: add defrag logic for sets #3833
Conversation
Signed-off-by: kostas <[email protected]>
| const size_t blob_len = intsetBlobLen(is); | ||
| intset* replacement = (intset*)zmalloc(blob_len); | ||
| memcpy(replacement, is, blob_len); |
There was a problem hiding this comment.
This is based on my understanding of how intset is implemented. I could be wrong.
- I can test this locally that memcpy works so we don't run into trouble
- Are there any tests that hit this path @adiholden
| return {is, false}; | ||
|
|
||
| const size_t blob_len = intsetBlobLen(is); | ||
| intset* replacement = (intset*)zmalloc(blob_len); |
There was a problem hiding this comment.
we are allocating here another set with same size, but what if this set is very big? it can cause spike in rss.
for listpack its ok because we use it for small hsets
There was a problem hiding this comment.
Imo I thought the exact same thing but we do the same for other DefragLogic. Maybe we can do this as an improvement for a future PR? That is, do not Defrag relateively big data structures ?
There was a problem hiding this comment.
what other defrag logic will ? as I wrote for listpack it is not risk because it is limited in size,
ReallocIfNeeded in StrMap is itereating on all elements so we are not allocating twice the size of the entire map right? regarding string type reallocation I dont think thats an issue because I dont think uses use huge strings thats probably not a real use case
There was a problem hiding this comment.
ReallocIfNeeded in StrMap is itereating on all elements so we are not allocating twice the size of the entire map right?
True
Intsets
We have a limit on our side at 256 elements so I guess it should be fine. We also serialize a string to long, so I guess no more than 8 bytes per entry. In total we are looking at 256 * 8 bytes which is not a lot 🤷
There was a problem hiding this comment.
oh inset is also limited with number of elements that in this case there is no issue
This PR adds defragmentation logic for sets.
partially resolves #3822 [1/2]