Optimize range deletion in zset encoded with listpack #13132

lyq2333 · 2024-03-13T07:17:02Z

In past, if we delete a range of elements in zset encoded with listpack, we call lpDeleteRangeWithEntry repeatedly. But lpDeleteRangeWithEntry calls lpShrinkToFit in which realloc is called. This means we call many unnecessary realloc.

In this PR, we optimize zremrangeby[score|lex] and zpop[min|max] by deleting all elements once to avoid redundant realloc. We add a new function named lpDeleteRangeWithEntryPtr to delete the range of elements from the start address to the tail address and avoid duplicate traversal search.

The result shows that the performance of zremrangeby[score|lex] can improve about 70% and the performance of zpop[min|max] can improve about 8%.

zremrangebyscore

I set 500,000 zset keys and each has 100 elements. Then I use zremrangebyscore to delete all. The result is as follow. The performance of range deletion increases about 70%.
Unstable

Summary:
  throughput summary: 39326.73 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        1.180     0.280     1.167     1.591     2.375     4.447

This PR

Summary:
  throughput summary: 68101.34 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        0.658     0.160     0.639     0.871     0.927     3.063

zpopmin

zpop[min|max] has the same problem. I set 500,000 zset keys and each has 100 elements. Then I use zpopmin to delete all. The result is as follow. The performance of range deletion increases about 8%.
Unstable

Summary:
  throughput summary: 21452.78 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        1.894     0.696     1.847     2.743     2.935     4.855

This PR

Summary:
  throughput summary: 23104.83 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        1.105     0.528     1.095     1.167     1.511     4.471

lyq2333 · 2024-03-13T07:37:04Z

@sundb I'd appreciate it if you could review this pr.

sundb

LGTM

src/t_zset.c

lyq2333 · 2024-03-13T08:36:57Z

It seems that our sanitizer CI has some problems.

sundb · 2024-03-13T08:47:20Z

@lyq2333 it's nothing to do with this PR, i found it yesterday but don't know why.

soloestoy · 2024-03-15T07:12:41Z

src/t_zset.c

        } else {
            /* No longer in range. */
            break;
        }
    }
-
+    if (num)
+        zl = lpDeleteRangeWithEntry(zl, &first_ele, 2 * num);


After the above loop, we can get the range's tail, which is eptr (if NULL it means it's the last one in listpack). I think we could implement another function lpDeleteRangeWithEntryPtr(unsigned char *lp, unsigned char **first, unsigned char **tail) to avoid the traversal search for tail within lpDeleteRangeWithEntry.

In this case we also need to pass the entry number between first and tail, otherwise we can't update the entry numbers, but that also make this method a little odd.

I find we have the same problem in zpopmin/max with count. In zpopmin/max, we also get the start position, end position and the number elements to be removed. Shall I add a new function like lpDeleteRangeWithEntryPtr to solve both of them?

@lyq2333 we can go ahead with this way, we can compromise on it if it really brings a boost.

@sundb @soloestoy I add the new function lpDeleteRangeWithEntryPtr and make a test. The result is as follow. It improve about 6% based on the origin branch(not unstable).

Before

Summary: throughput summary: 64333.51 requests per second latency summary (msec): avg min p50 p95 p99 max 0.700 0.152 0.679 0.943 1.023 2.279

Now

Summary: throughput summary: 68101.34 requests per second latency summary (msec): avg min p50 p95 p99 max 0.658 0.160 0.639 0.871 0.927 3.063

I also make a test about zpopmin. I put the result in the top comment. The performance improvement is about 8% because we costs many time on reply.

soloestoy · 2024-03-18T08:58:23Z

src/t_zset.c

@@ -4035,11 +4062,11 @@ void genericZpopCommand(client *c, robj **keyv, int keyc, int where, int emitkey
            serverAssertWithInfo(c,zobj,zln != NULL);
            ele = sdsdup(zln->ele);
            score = zln->score;
+            serverAssertWithInfo(c,zobj,zsetDel(zobj,ele));


we can call zslDeleteRangeByRank after the whole fake pop loop.

soloestoy · 2024-03-18T09:05:27Z

src/t_zset.c

            serverAssertWithInfo(c,zobj,sptr != NULL);
            score = zzlGetScore(sptr);
+            /* In previous versions, we call notifyKeyspaceEvent after the first entry is deleted. 


I think we can move the notifyKeyspaceEvent to after all the pop completed. This way, we won't need to check the result_count, and the logic will be much clearer. Commands like zremrange* operate in the same manner since the execution of the command is atomic.

module is a bit special, but I believe that moving the notification from after the first element is popped to after all elements have been popped will not break compatibility with modules.

I'd also like to ping @oranagra to confirm this.

I also think notification immediately after removing the first element is weird. But I don't know the reason.

indeed the logic of firing the notification only on the first removal seems odd (rather than firing on every removal).
when we realize that when this code was created in 56bbab2 there were no modules, then firing it on the first removal was equivalent to firing it at the end (if there was at least one removal).
but with modules it makes less sense.
i'd vote to move this to the end and document the behavior change for modules.
@MeirShpilraien WDYT?

soloestoy · 2024-03-18T09:08:05Z

src/t_zset.c

@@ -4035,11 +4062,11 @@ void genericZpopCommand(client *c, robj **keyv, int keyc, int where, int emitkey
            serverAssertWithInfo(c,zobj,zln != NULL);
            ele = sdsdup(zln->ele);


Another minor optimization: for the skiplist, we don't need to copy the ele. Or perhaps we should change it so that listpack and skiplist handle their own loops respectively.

sundb · 2024-03-18T09:14:16Z

src/listpack.c

+    unsigned long poff = *first-lp;
+
+    /* Move tail to the front of the listpack */
+    memmove(*first, tail, eofptr - tail + 1);


i'm worried about about this, if the bytes is corrupted, does this mean it's possible to use eofptr to read the memory data after this listpack?

I guess that one reason of the performance improvement is the reduction calls of lpAssertValidEntry.

In lpDeleteRangeWithEntry, if the bytes is corrupted, we also have the same problem. If we want to avoid this problem, I think maybe we have to travel the whole listpack to verify the validity of the bytes.

We get first and tail by lpNext or lpPrev, in which we have called lpAssertValidEntry. I think maybe we don't need to repeat it in lpDeleteRangeWithEntryPtr.

yeah, you're probably right, please merge the unstable and i'll run a fully CI to check it.

sundb · 2024-03-18T13:15:04Z

@lyq2333 please have a look at these crash in the following fully CI:
https://github.com/sundb/redis/actions/runs/8325132244/job/22781563626
https://github.com/sundb/redis/actions/runs/8325132244/job/22778109249
please ignore this, I saw it in other places.

lyq2333 · 2024-03-19T02:45:59Z

@lyq2333 please have a look at these crash in the following fully CI: https://github.com/sundb/redis/actions/runs/8325132244/job/22781563626 https://github.com/sundb/redis/actions/runs/8325132244/job/22778109249 please ignore this, I saw it in other places.

@sundb I think maybe this is because #13154. PTAL.

CLAassistant · 2024-03-24T23:02:14Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

lyq2333 added 2 commits March 13, 2024 14:43

optimize zset range delete

78832d6

Merge branch 'unstable' into optimize-zset-range

c2c6b32

sundb reviewed Mar 13, 2024

View reviewed changes

src/t_zset.c Outdated Show resolved Hide resolved

src/t_zset.c Outdated Show resolved Hide resolved

sundb reviewed Mar 13, 2024

View reviewed changes

src/t_zset.c Outdated Show resolved Hide resolved

src/t_zset.c Outdated Show resolved Hide resolved

Update src/t_zset.c

a6678fb

sundb previously approved these changes Mar 13, 2024

View reviewed changes

soloestoy reviewed Mar 15, 2024

View reviewed changes

lyq2333 added 3 commits March 18, 2024 10:31

add new lpDeleteRangeWithEntryPtr

7eb48ad

optimize zpopmax/min

5f2ccf4

Merge branch 'unstable' into optimize-zset-range

f53fb19

soloestoy reviewed Mar 18, 2024

View reviewed changes

sundb reviewed Mar 18, 2024

View reviewed changes

lyq2333 added 2 commits March 19, 2024 15:12

refactor

fdc01bb

add comment

53ce9dc

lyq2333 dismissed sundb’s stale review via 53ce9dc April 1, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize range deletion in zset encoded with listpack #13132

Optimize range deletion in zset encoded with listpack #13132

lyq2333 commented Mar 13, 2024 •

edited

lyq2333 commented Mar 13, 2024

sundb left a comment

lyq2333 commented Mar 13, 2024

sundb commented Mar 13, 2024

soloestoy Mar 15, 2024

sundb Mar 15, 2024

lyq2333 Mar 18, 2024

sundb Mar 18, 2024

lyq2333 Mar 18, 2024 •

edited

lyq2333 Mar 18, 2024

soloestoy Mar 18, 2024

soloestoy Mar 18, 2024

lyq2333 Mar 18, 2024

oranagra Mar 19, 2024

soloestoy Mar 18, 2024

sundb Mar 18, 2024

sundb Mar 18, 2024

lyq2333 Mar 18, 2024 •

edited

sundb Mar 18, 2024

sundb commented Mar 18, 2024 •

edited

lyq2333 commented Mar 19, 2024 •

edited

CLAassistant commented Mar 24, 2024

		@@ -4035,11 +4062,11 @@ void genericZpopCommand(client c, robj *keyv, int keyc, int where, int emitkey
		serverAssertWithInfo(c,zobj,zln != NULL);
		ele = sdsdup(zln->ele);

Optimize range deletion in zset encoded with listpack #13132

Are you sure you want to change the base?

Optimize range deletion in zset encoded with listpack #13132

Conversation

lyq2333 commented Mar 13, 2024 • edited

zremrangebyscore

zpopmin

lyq2333 commented Mar 13, 2024

sundb left a comment

Choose a reason for hiding this comment

lyq2333 commented Mar 13, 2024

sundb commented Mar 13, 2024

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

lyq2333 Mar 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyq2333 Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundb commented Mar 18, 2024 • edited

lyq2333 commented Mar 19, 2024 • edited

CLAassistant commented Mar 24, 2024

lyq2333 commented Mar 13, 2024 •

edited

lyq2333 Mar 18, 2024 •

edited

lyq2333 Mar 18, 2024 •

edited

sundb commented Mar 18, 2024 •

edited

lyq2333 commented Mar 19, 2024 •

edited