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

maybe an optimizable point for zadd operation #5179

Closed
pyloque opened this issue Jul 28, 2018 · 10 comments
Closed

maybe an optimizable point for zadd operation #5179

pyloque opened this issue Jul 28, 2018 · 10 comments

Comments

@pyloque
Copy link

pyloque commented Jul 28, 2018

When we use zadd command to update the score for exists value,if the score delta is small,element rank will be no change,i think we only need change the score, while deleting and reinserting is unnecessary.

To be sure the rank is not changed, we can use current node's forward and backward pointer to access the sibling nodes, to test whether the new score is still between the score of prev and next node.

The following source code is clipped from current redis repo.

int zsetAdd(robj *zobj, double score, sds ele, int *flags, double *newscore) {
...
            /* Remove and re-insert when score changes. */
            if (score != curscore) {
                zskiplistNode *node;
                serverAssert(zslDelete(zs->zsl,curscore,ele,&node));
                znode = zslInsert(zs->zsl,score,node->ele);
                /* We reused the node->ele SDS string, free the node now
                 * since zslInsert created a new one. */
                node->ele = NULL;
                zslFreeNode(node);
                /* Note that we did not removed the original element from
                 * the hash table representing the sorted set, so we just
                 * update the score. */
                dictGetVal(de) = &znode->score; /* Update score ptr. */
                *flags |= ZADD_UPDATED;
            }
...
@pyloque pyloque changed the title maybe an optimize point for zadd operation maybe an optimizable point for zadd operation Jul 28, 2018
@antirez
Copy link
Contributor

antirez commented Aug 1, 2018

Very interesting optimization! I just wrote an implementation, stress testing it and committing tomorrow.

antirez added a commit that referenced this issue Aug 1, 2018
When the element new score is the same of prev/next node, the
lexicographical order kicks in, so we can safely update the node in
place only when the new score is strictly between the adjacent nodes
but never equal to one of them.

Technically speaking we could do extra checks to make sure that even if the
score is the same as one of the adjacent nodes, we can still update on
place, but this rarely happens, so probably not a good deal to make it
more complex.

Related to #5179.
@antirez
Copy link
Contributor

antirez commented Aug 1, 2018

P.S. Just pushed on faster-zadd branch here at Github.

@antirez
Copy link
Contributor

antirez commented Aug 2, 2018

UPDATE: I'm merging the change, because I can measure a speed boost of more than 10% in an use case which is not even optimal to stress the optimization well enough, so it looks like it's worth it. Thanks @pyloque for the hint.

antirez added a commit that referenced this issue Aug 2, 2018
When the element new score is the same of prev/next node, the
lexicographical order kicks in, so we can safely update the node in
place only when the new score is strictly between the adjacent nodes
but never equal to one of them.

Technically speaking we could do extra checks to make sure that even if the
score is the same as one of the adjacent nodes, we can still update on
place, but this rarely happens, so probably not a good deal to make it
more complex.

Related to #5179.
JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Dec 17, 2018
JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Dec 17, 2018
When the element new score is the same of prev/next node, the
lexicographical order kicks in, so we can safely update the node in
place only when the new score is strictly between the adjacent nodes
but never equal to one of them.

Technically speaking we could do extra checks to make sure that even if the
score is the same as one of the adjacent nodes, we can still update on
place, but this rarely happens, so probably not a good deal to make it
more complex.

Related to redis#5179.
@daydayup825
Copy link

hello da lao

@liwen2555376
Copy link

nice!

@greedycat120
Copy link

👍

@LyreBoss
Copy link

LyreBoss commented Aug 9, 2020

🐂

@oranagra
Copy link
Member

oranagra commented Aug 9, 2020

i see the code that implements it is already merged. closing.

@oranagra oranagra closed this as completed Aug 9, 2020
@blankjee
Copy link

👍

@radiocontroller
Copy link

👍

pulllock pushed a commit to pulllock/redis that referenced this issue Jun 28, 2023
pulllock pushed a commit to pulllock/redis that referenced this issue Jun 28, 2023
When the element new score is the same of prev/next node, the
lexicographical order kicks in, so we can safely update the node in
place only when the new score is strictly between the adjacent nodes
but never equal to one of them.

Technically speaking we could do extra checks to make sure that even if the
score is the same as one of the adjacent nodes, we can still update on
place, but this rarely happens, so probably not a good deal to make it
more complex.

Related to redis#5179.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants