Skip to content
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

Fix several bugs in quicklist: #12568

Closed
wants to merge 4 commits into from
Closed

Fix several bugs in quicklist: #12568

wants to merge 4 commits into from

Conversation

imchuncai
Copy link

  • As is disscussed in Minor comment bug #12548: Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and quicklistInsertAfter() will cause a packed node violate size limit.
  • As is disscussed in [BUG] quicklist compress bug #12563: A node will not be compressed if it is not compress small enough. So node's member recompress will stay 0 after calling function quicklistDecompressNodeForUse(). If that node's entry is changed later, call function quicklistRecompressOnly() will not make that node compressed. In this situation, we should call function quicklistCompress() instead, I will take this approach in this commit, obviously it's not efficient. We should redesign 'recompress' to fundamentally solve the problem.
  • struct quicklistNode's member dont_compress is removed.

- As is disscussed in #12548:
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore()
  and quicklistInsertAfter() will cause a packed node violate size limit.
- As is disscussed in #12563:
  A node will not be compressed if it is not compress small enough.
  So node's member recompress will stay 0 after calling function
  quicklistDecompressNodeForUse(). If that node's entry is changed later,
  call function quicklistRecompressOnly() will not make that node compressed.
  In this situation, we should call function quicklistCompress() instead,
  I will take this approach in this commit, obviously it's not efficient.
  We should redesign 'recompress' to fundamentally solve the problem.
- struct quicklistNode's member dont_compress is removed.
Comment on lines +747 to +748
node->entry = lpReplace(node->entry, &entry->zi, data, sz);
quicklistNodeUpdateSz(node);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid these changes for a better review.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's related to #12548.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that we can revert unrelated changes.
It's like these two lines that don't need to be changed.

        entry->node->entry = lpReplace(entry->node->entry, &entry->zi, data, sz);
        quicklistNodeUpdateSz(entry->node);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. Don't you think ‘entry->node->entry’ is kind of weird? And 'entry->node' is used everywhere in function quicklistReplaceEntry(), take it out can improve readability of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your idea, but for the sake of better review, we can avoid so many changes, maybe you can make these changes at the end instead of now!

Copy link
Collaborator

@sundb sundb Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also handle this and run ./runtest once.
I see that the runtest is also failed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed runtest is fixed.

_quicklistListpackMerge(quicklist, target, target->next);
}
unsigned int newCount = a->count + b->count;
lpMerge(&a->entry, &b->entry);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe why this changes was made?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/* else, the merge returned NULL and nothing changed. */
This comment is wrong, something is changed which is a and b is decompressed.
This else is not unnecessary or we should recompress a and b.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function _quicklistMergeNodes() is deleted, don't used it now.

src/quicklist.c Outdated
@@ -1171,7 +1147,7 @@ int quicklistDelRange(quicklist *quicklist, const long start,
quicklist->count -= del;
quicklistDeleteIfEmpty(quicklist, node);
if (node)
quicklistRecompressOnly(node);
quicklistCompress(quicklist, node);
Copy link
Collaborator

@sundb sundb Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using quicklistCompress here and in other places? Because node->recompress is 0 and can't compress?
This change may hide other bugs where node is not compressed.
btw: quicklistCompress is more expensive than quicklistRecompressOnly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mentioned in commit log, This change can fix #12563, for more efficient experience, quicklistNode->recompress should be redesigned.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that quicklistDelRange also can cause uncompress nodes?
Could you provide the smoke test and others?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this means I can't say that quicklistDelRange() won't cause uncompressed nodes. If anyone can prove that, should leave a comment here and keep using quicklistRecompressOnly().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the uncompressed node is only created because we reset iter->node early before quicklistReleaseIterator(), not because of the uncompression caused by quicklistRecompressOnly.
I'd prefer to fix the problem of creating uncompressed nodes, rather than using quicklistRecompressOnly for all potentially uncompressed nodes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first scenario, I don't know if it's possible that a uncompressed node can be compressed after deleted part of the data.

Copy link
Collaborator

@sundb sundb Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if so we should find out why this node wasn't compressed correctly.
at any time we should assume that a node is compressed correctly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This node is compressed correctly, but it is not compress small enough, so it remains uncompressed. What I don't know is if it's possible to compress it small enough after deleted part of the data.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compression algorithms are usually unlikely to do this, when a piece of data has a low compression ratio, it means that there are too few duplicate blocks, and when some of the blocks are removed, its compression ratio should become lower.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, Then I will roll back this change.

quicklistNodeUpdateSz(new_node);
__quicklistInsertNode(quicklist, node, new_node, after);
_quicklistMergeNodes(quicklist, node);
_quicklistInsertIntoFullNode(quicklist, entry, value, sz, after);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why adding this method?
It seems that it doesn't relate to #12563.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's related to #12548.

Copy link
Author

@imchuncai imchuncai Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append value to new_node without check can make listpack too big.

@sundb
Copy link
Collaborator

sundb commented Sep 11, 2023

@imchuncai Thanks, Can you also add some unit tests at the bottom of quicklist.c?

@sundb
Copy link
Collaborator

sundb commented Sep 12, 2023

Please also use ./runtest or ./runtest --single unit/type/list --large-memory.
This pr fails in my local PC.

[err]: Test LSET with packed / plain combinations in tests/unit/type/list.tcl
Expected 'bb' to be equal to 'ddddddddddddddddddddddddddddddd...'

- fix test bug
- add unit tests for listpack limit test
- roll back changes for function quicklistDelRange()
@imchuncai
Copy link
Author

@imchuncai Thanks, Can you also add some unit tests at the bottom of quicklist.c?

Added.

@imchuncai
Copy link
Author

Please also use ./runtest or ./runtest --single unit/type/list --large-memory. This pr fails in my local PC.

[err]: Test LSET with packed / plain combinations in tests/unit/type/list.tcl
Expected 'bb' to be equal to 'ddddddddddddddddddddddddddddddd...'

Fixed

src/quicklist.c Outdated
Comment on lines 769 to 770
iter->offset--;
quicklistNext(iter, entry);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should engage in these dangerous behaviors.
quicklistNext() should be the responsibility of the caller of the iterator, and should not be used internally.
All we can do is update the iterator to the correct position when it changes or reset it to prevent it from being used again.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's removed, it causes bug anyway.

@@ -1017,14 +1000,14 @@ REDIS_STATIC void _quicklistInsert(quicklistIter *iter, quicklistEntry *entry,
node->entry = lpInsertString(node->entry, value, sz, entry->zi, LP_AFTER, NULL);
node->count++;
quicklistNodeUpdateSz(node);
quicklistRecompressOnly(node);
quicklistCompress(quicklist, node);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar changes in this file should be handled as we discussed in #12568 (comment).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar but not same, I've added a new unit test: TEST("small listpack compress"). Try this.

- fix test bug
- add new unit test for small listpack compress
- fix another issue in function _quicklistInsert() whitch not update
  node's sz
@sundb
Copy link
Collaborator

sundb commented Sep 15, 2023

@imchuncai I see that you changed the positions of quicklistReplaceEntry() and quicklistReplaceAtIndex(), which will cause a lot of changes and be difficult to review.

@sundb
Copy link
Collaborator

sundb commented Sep 15, 2023

@imchuncai IMHO, we can fix #12563 simply by using following patch:

diff --git a/src/quicklist.c b/src/quicklist.c
index 301a2166..8bd15d60 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -1071,12 +1071,14 @@ REDIS_STATIC void _quicklistInsert(quicklistIter *iter, quicklistEntry *entry,
         quicklistNodeUpdateSz(new_node);
         __quicklistInsertNode(quicklist, node, new_node, after);
         _quicklistMergeNodes(quicklist, node);
+        node = NULL;
     }
 
     quicklist->count++;
 
     /* In any case, we reset iterator to forbid use of iterator after insert.
      * Notice: iter->current has been compressed in _quicklistInsert(). */
+    if (node) quicklistCompress(quicklist, node);
     resetIterator(iter); 
 }

The reason why this node doesn't compress is that we forgot to compress the iterator node before resetting the iterator.
All we have to do is just do the same things as quicklistReleaseIterator().

@imchuncai
Copy link
Author

imchuncai commented Sep 15, 2023

@imchuncai IMHO, we can fix #12563 simply by using following patch:

diff --git a/src/quicklist.c b/src/quicklist.c
index 301a2166..8bd15d60 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -1071,12 +1071,14 @@ REDIS_STATIC void _quicklistInsert(quicklistIter *iter, quicklistEntry *entry,
         quicklistNodeUpdateSz(new_node);
         __quicklistInsertNode(quicklist, node, new_node, after);
         _quicklistMergeNodes(quicklist, node);
+        node = NULL;
     }
 
     quicklist->count++;
 
     /* In any case, we reset iterator to forbid use of iterator after insert.
      * Notice: iter->current has been compressed in _quicklistInsert(). */
+    if (node) quicklistCompress(quicklist, node);
     resetIterator(iter); 
 }

The reason why this node doesn't compress is that we forgot to compress the iterator node before resetting the iterator. All we have to do is just do the same things as quicklistReleaseIterator().

The reason is we changed node->entry, that makes node->recompress no longer reliable. Poor design of node->recompress is the fundamental problem.
The reason why I replaced quicklistRecompressOnly() with quicklistCompress() is quicklistRecompressOnly() didn't do the job he supposed to to.

@sundb
Copy link
Collaborator

sundb commented Sep 15, 2023

@imchuncai But we recompress node by using quicklistCompress() not quicklistRecompressOnly().
Please refer the using of quicklistCompress() in the origin quicklistReplaceEntry().

@imchuncai
Copy link
Author

@imchuncai But we recompress node by using quicklistCompress() not quicklistRecompressOnly(). Please refer the using of quicklistCompress() in the origin quicklistReplaceEntry().

So I replaced quicklistRecompressOnly() with quicklistCompress(), what's the problem?

@sundb
Copy link
Collaborator

sundb commented Sep 15, 2023

@imchuncai Yeah, you are right, this is exactly how it's handled in quicklistReplaceEntry().

@imchuncai
Copy link
Author

We should redesign node->recompress, if we only changed node->entry, quicklistRecompressOnly() should always work.

@imchuncai
Copy link
Author

Replace quicklistRecompressOnly() with quicklistCompress() is just a quick fix not efficient。

@sundb
Copy link
Collaborator

sundb commented Sep 15, 2023

@imchuncai Thanks, I finally understand the desing problem you mentioned about recompress.
If a node wasn't compressed before changing it, but it can be compressed after changing it.
now quicklistRecompressOnly() won't compress it that point.

Comment on lines +958 to 959
quicklistCompress(quicklist, new_node);
quicklistRecompressOnly(node);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
quicklistCompress(quicklist, new_node);
quicklistRecompressOnly(node);
quicklistCompressNode(new_node);
quicklistCompress(node);

Maybe it's more appropriate.
Since the newnode has data added to it, we'll recompress it anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, new_node may not in the compress depth. And node->entry is not changed, node->recompress is reliable.

@sundb
Copy link
Collaborator

sundb commented Sep 15, 2023

@imchuncai I think we can fix both bugs in separate PRs, to avoid too many changes.
Please also handle the changes mentioned in #12568 (comment)

src/quicklist.c Outdated
Comment on lines 1035 to 1045
/* node->count > 1, node will not be removed */
quicklistDelEntry(iter, entry);
if (quicklistNext(iter, entry)) {
_quicklistInsert(iter, entry, data, sz, iter->direction == AL_START_HEAD);
} else {
int direction = iter->direction == AL_START_HEAD
? AL_START_TAIL : AL_START_HEAD;
quicklistIter *rev_iter = quicklistGetIterator(quicklist, direction);
quicklistNext(rev_iter, entry);
_quicklistInsert(rev_iter, entry, data, sz, direction == AL_START_HEAD);
quicklistReleaseIterator(rev_iter);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say I like this way that nested use of iterators.
Can you explain why we have to do as this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't insert first now, because entry->node have a chance be freed due to merge.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I'd prefer an implementation like _quicklistInsert to reimplement the replacement, rather than nested iterators.
But that would introduce a lot of duplicate code and more complexity.

@imchuncai
Copy link
Author

@imchuncai I see that you changed the positions of quicklistReplaceEntry() and quicklistReplaceAtIndex(), which will cause a lot of changes and be difficult to review.

What's your suggestion? Move it back and add declaration of function _quicklistInsert() above it?

@sundb
Copy link
Collaborator

sundb commented Sep 16, 2023

Move them back to their original position for better review.

@imchuncai
Copy link
Author

Move them back to their original position for better review.

done

@sundb
Copy link
Collaborator

sundb commented Sep 18, 2023

This PR fix actually equates packed_threshold to either packed_threshold or SIZE_SAFETY_LIMIT.
When an entry is larger than packed_threshold[fill] or SIZE_SAFETY_LIMIT,
it will always be kept in an isolated listpack, similar to a PLAIN node but with a listpack header.

I was thinking that maybe we could fix it in some other way.

  1. make quicklist break the maximum limit in some cases, i.e. a node's size can be up to optimization_level[4] + packed_threshold, which is what the origin code is doing.
  2. replace the existing packed_threshold with optimization_level or SIZE_SAFETY_LIMIT, i.e. an entry larger than this threshold will be a plain node.

I'm not sure which way is better.
Please share your thoughts, @imchuncai @oranagra

@imchuncai
Copy link
Author

This PR fix actually equates packed_threshold to either packed_threshold or SIZE_SAFETY_LIMIT. When an entry is larger than packed_threshold[fill] or SIZE_SAFETY_LIMIT, it will always be kept in an isolated listpack, similar to a PAINT node but with a listpack header.

I was thinking that maybe we could fix it in some other way.

  1. make quicklist break the maximum limit in some cases, i.e. a node's size can be up to optimization_level[4] + packed_threshold, which is what the origin code is doing.
  2. replace the existing packed_threshold with optimization_level or SIZE_SAFETY_LIMIT, i.e. an entry larger than this threshold will be a plain node.

I'm not sure which way is better. Please share your thoughts, @imchuncai @oranagra

It's not related to this PR. This PR fixed a packed node violate size limit, what the limitation is nor which node should be treated as a large element is not concerned.

@sundb
Copy link
Collaborator

sundb commented Sep 18, 2023

@imchuncai The scenarios I propose are only meant to avoid the implementations mentioned in #12568 (comment).
If we use SIZE_SAFETY_LIMIT or optimization_level[fill] instead of packed_threshold.
Consider the following scenarios

  1. When an entry is inserted that is smaller than SIZE_SAFETY_LIMIT, we allow the node to break the maximum limit, so its replacement size could be <SIZE_SAFETY_LIMIT*2.
  2. when the inserted entry is larger than SIZE_SAFETY_LIMIT, we treat it as a large element in the old way, because the entry will always exist alone anyway.

This was linked to issues Sep 20, 2023
@oranagra
Copy link
Member

disclaimer: i'm not certain i understand everything, as i only took a quick look at the code, and only read the top comment and last 3 at the top.

i understand that there's an overlapping between the threshold for PLAIN nodes, and the threshold of packed nodes with just one element (in case nodes are limited by count rather than size).

as far as i remember, SIZE_SAFETY_LIMIT is actually not about safety but an optimization, i.e. we on't want memmoves and reallocs when we're dealing with large items.

that said, this mechanism was created before PLAIN nodes existed, and i think that now that we have them, maybe we can reduce the default packed_threshold to 8kb, and there's no reason why we'd ever want to solve listpacks of just one element if we never gonna consider adding anything to that listpack.

i do think that we should take this opportunity to clean that code and simplify it (not just fix the bug), since this PR looks quite large anyway.
i.e. if it was just a 2 lines fix, i'd be ok to merge it as a bug fix, and leave the cleanup for later.

while on that subject, considering the amount of changes, and the fact the bugs aren't critical ones (don't really affect redis's behavior, right?), i don't think we should backport this fix to any existing release.
can we remove the backport tags?

@sundb
Copy link
Collaborator

sundb commented Sep 26, 2023

while on that subject, considering the amount of changes, and the fact the bugs aren't critical ones (don't really affect redis's behavior, right?), i don't think we should backport this fix to any existing release. can we remove the backport tags?

I don't think we need to backport this which doesn't do any harm.

@imchuncai
Copy link
Author

I'm not able to contributing code anymore.

@imchuncai imchuncai closed this Oct 2, 2023
@sundb
Copy link
Collaborator

sundb commented Oct 2, 2023

@imchuncai Still appreciate the effort you put into it.

imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
imchuncai pushed a commit to imchuncai/redis that referenced this pull request Dec 12, 2023
Why:
I tried to solve the issue I found earlier, but found myself stuck in a quagmire
because the issues kept coming up while I fix the old one, so I finally decided
to rewrite it.

Issues with the old one:
- A node which should be compressed stays raw
  This is due to by poor design of quicklist->recompress, the design forgot the
  situation that a node could stay uncompressed if it can not compress small
  enough. And if we changed the node, wo should perform compress on it again.
  See issue redis#12563.
- Iterator don't behave like iterator
  Iterator will be reset and not avaliable for further use after replace or
  insert, see marcro resetIterator(). The only operation that athe iterator does
  not get reset is quicklistDelEntry(), and it has a commment about it, but the
  comment is wrong, the iterator may not behave like the comment says.
  See issue redis#12614.
- Packed node violate size limit
  Certen call to function quicklistReplaceEntry(), quicklistInsertBefore() and
  quicklistInsertAfter() will cause a packed node violate size limit.
  See issue redis#12548.
- Merge operation only performed in insert
  There is no merging in delete nor replace, which can make the quicklist
  contain adjacent small nodes.
  See issue redis#12856.
- Algorithms that maintain compress depth are not efficient
  the algorithm to maintain compress depth after add or delete is to check nodes
  on both sides of the list, time complexity is O(n), where n is the uncompressed
  depth on both sides of the list.

All the changes:
- Partition the node
  Divide the node into three partitions: head, middle and tail. The head and
  tail partitions hold uncompressed nodes, and the middle partition holds
  compressed nodes. Therefore,the time complexity of maintaining compress depth
  after adding or deleting a node will drop to O(1), moving at most one node
  from one partition to another.
- Removed annoying members recompress, attempted_compress and dont_compress from
  quicklist node structure
- Merge structure quicklistIter and quicklistEntry
- The historical parameter packed_threshold has been removed
  This is mentioned by @sundb and @oranagra in pull request redis#12568.
- Merge strategy is added
  That is that any adjacent node in quicklist can not be merged.
@imchuncai imchuncai mentioned this pull request Dec 12, 2023
oranagra pushed a commit that referenced this pull request Feb 22, 2024
Following #12568

In issue #9357, when inserting an element larger than 1GB, we currently
store it in a plain node instead of a listpack.
Presently, when we insert an element that exceeds the maximum size of a
packed node, it cannot be accommodated in any other nodes, thus ending
up isolated like a large element.
I.e. it's a node with only one element, but it's listpack encoded rather
than a plain buffer.

This PR lowers the threshold for considering an element as 'large' from
1GB to the maximum size of a node.
While this change doesn't completely resolve the bug mentioned in the
previous PR, it does mitigate its potential impact.

As a result of this change, we can now only use LSET to replace an
element with another element that falls below the maximum size
threshold.
In the worst-case scenario, with a fill of -5, the largest packed node
we can create is 2GB (32k * 64k):
* 32k: The smallest element in a listpack is 2 bytes, which allows us to
store up to 32k elements.
* 64k: This is the maximum size for a single quicklist node.

## Others
To fully fix #9357, we need more work, as discussed in #12568, when we
insert an element into a quicklistNode, it may be created in a new node,
put into another node, or merged, and we can't correctly delete the node
that was supposed to be deleted.
I'm not sure it's worth it, since it involves a lot of modifications.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

[BUG] quicklist compress bug Minor comment bug
3 participants