Optimize skiplist query efficiency by embedding the skiplist header

[chzhoo]

Description

Embedding the skiplist header reduces memory jumps, thus optimizing sorted set query speed.

// Before
typedef struct zskiplist {
        struct zskiplistNode *header, *tail;
        unsigned long length;
        int level; /* Height of the skiplist. */
} zskiplist;


// After
typedef struct zskiplist {
        unsigned long length; 
        struct zskiplistNode *tail;
        /* Since the span at level 0 is always 1 or 0 , 
         * this field is instead used for storing the height of the skiplist. */
        struct zskiplistLevel {
                struct zskiplistNode *forward;
                unsigned long span;
        } level[ZSKIPLIST_MAXLEVEL];
} zskiplist;

Benchmark

Step 1

Generate the test data using the following lua script and cli command

lua script

local start_idx = 0
local end_idx = tonumber(ARGV[1])
local elem_count = 129 -- Skiplist storage is activated when element count surpasses 128

for i = start_idx, end_idx do
    local key = "zset:" .. string.format("%012d", i)
    local members = {}

    for j = 0, elem_count - 1 do
        table.insert(members, j)
        table.insert(members, "member:" .. j)
    end

    redis.call("ZADD", key, unpack(members))
end

return "OK: Created " .. (end_idx - start_idx + 1) .. " zsets"

cli command

valkey-cli eval "$(cat load.lua)" 0 $ZSET_COUNT

Step 2

Run benchmark 5 times through the following command and select the peak value as the result

valkey-benchmark -n 5000000 -r $ZSET_COUNT --threads 2 zrange zset:__rand_int__ 0 0

Benchmark result

ZSET_COUNT	QPS before optimization	QPS after optimization	Performance Boost
10000	293875.62	294100.34	0.0%
40000	270241.03	277531.06	2.7%
100000	249775.20	259713.27	4.0%
1000000	224678.72	235260.91	4.7%
2000000	219751.23	229863.91	4.6%

Benchmark Env

CPU: AMD EPYC 9K65 192-Core Processor * 8
OS: Ubuntu Server 24.04 LTS 64bit
Memory: 32GB
VM: Tencent Cloud | SA9.2XLARGE32

[codecov]

Codecov Report

❌ Patch coverage is 95.68966% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.42%. Comparing base (8ea7f13) to head (47dc765).
⚠️ Report is 7 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/object.c	50.00%	3 Missing ⚠️
src/t_zset.c	97.95%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2867      +/-   ##
============================================
- Coverage     72.44%   72.42%   -0.03%     
============================================
  Files           128      128              
  Lines         70415    70491      +76     
============================================
+ Hits          51011    51050      +39     
- Misses        19404    19441      +37     

Files with missing lines	Coverage Δ
src/defrag.c	80.66% <100.00%> (-0.07%)	⬇️
src/lazyfree.c	85.41% <100.00%> (-7.07%)	⬇️
src/rdb.c	77.19% <100.00%> (-0.18%)	⬇️
src/server.h	100.00% <ø> (ø)
src/sort.c	94.82% <100.00%> (+0.01%)	⬆️
src/t_zset.c	96.76% <97.95%> (+0.01%)	⬆️
src/object.c	82.04% <50.00%> (ø)

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ranshid]

Nice!

I will do a deeper review, I just want to first eliminate concerns about potential future extensions of the skiplist metadata.

[ranshid]

I fear about this blocking us from changing/extending the skiplist metadata because it might later impact the size of every skiplist node.
Maybe an alternative would be to keep the zskiplistNode as is and embed a node as part of it like this:

typedef struct zskiplist {
    union {
          struct {
               unsigned long length; /* Number of elements in the skiplist */
               struct zskiplistNode *tail; /* Tail element of the skiplist */
          };
          struct zskiplistNode header;
    };
} zskiplist;

[zuiderkwast]

Good point Ran.

I'm not sure your suggestion works though, because it's not allowed to embed a struct with a "flexible array" member (the level[]) into another struct. I get a pendantic warning for this:

server.h:1450:26: warning: invalid use of structure with flexible array member [-Wpedantic]
 1450 |     struct zskiplistNode header;
      |                          ^~~~~~

To work around this warning, we could use a trick that was common before "flexible arrays" existed. That is, to declare an array of size 1 instead (level[1]) and still use it as a flexible array. We only need to take it into account when we do sizeof(zskiplistNode) but other than that, all should work the same and now we can include a zskiplistNode inside the zskiplist struct.

[ranshid]

Yeh. and another option is to keep the suggested typedef struct zskiplistNode zskiplist; for now and just make sure to provide zsl getter functions like:

zskiplistNode* zslGetHeader(zskiplist *zsl) {
        return (zskiplistNode*)(zsl);
}

zslGetLen(zskiplist *zsl) {
      return (unsigned long)(zslGetHeader(zsl)->score);
}

zskiplistNode* zslGetTail {
    return zslGetHeader(zsl)->backward;
}

etc...

in the future, in case we wish to extend the skiplist metadata we could just allocate the zskiplistNode with the zskiplist, eg:

typedef struct zskiplist {
        int something;

} zskiplist;

zskiplist *zslCreate(void) {
    int j;
    zskiplist *zsl;

    zsl = malloc(sizeof(*zsl) + sizeof(zskiplistNode) + ZSKIPLIST_MAXLEVEL * sizeof(struct zskiplistLevel));
...

[chzhoo]

Thanks all for the discussion. I understand the code needs the following changes:

1. Embed zskiplistNode in zskiplist. The struct will be defined as follows:

typedef struct zskiplistLevel {
         struct zskiplistNode *forward;
         unsigned long span;
} zkiplistLevel;

typedef struct zskiplistNode {
        double score;
        struct zskiplistNode *backward;
        zskiplistLevel level[1];
} zskiplistNode;

typedef struct zskiplist {
    union {
          struct zskiplistNode header;
          struct {
               unsigned long length;
               struct zskiplistNode *tail;
          };
    };
} zskiplist;

  For future extensions to the skiplist metadata, the following definition can be used:

/* The 'level' array is placed at the head of the struct (rather than the tail)
 * to ensure all metadata(length/tail/somemeta) is stored contiguously. */
typedef struct zskiplistNode {
        zskiplistLevel level[1];
        double score;
        struct zskiplistNode *backward;
} zskiplistNode;

typedef struct zskiplist {
        union {
                struct zskiplistNode header;
                struct {
                        char padding[sizeof(zskiplisLevel)];
                        unsigned long length;
                        struct zskiplistNode *tail;
        };
        int somemeta;
} zskiplist;

2. Provide zsl-related getter/setter functions, such as zslGetTail, zslGetLength, and zslGetHeader.

What do you think? @ranshid @zuiderkwast

[ranshid]

I think #2 was suggested as an alternative to #1 but IMO it would be great to have them both.

[zuiderkwast]

The level[] or level[1] needs to be the last member in the node struct, so that it can be variable size. If the node is embedded in another struct (zskiplist) it needs to be the last member there.

[zuiderkwast]

The original suggestion where zskiplist is a typedef of zskiplistNode is also fine by me, but yes with getters/setters it's better.

[chzhoo]

The level[] or level[1] needs to be the last member in the node struct, so that it can be variable size. If the node is embedded in another struct (zskiplist) it needs to be the last member there.

The following layout allows placing level array at the head of the node struct, though it requires more extensive changes and can be considered later.

+---------+-----+---------+-------+------------------+-------------+
| level-n | ... | level-0 | score | backward-pointer | element-sds |
+---------+-----+---------+-------+------------------+-------------+
                |
                | 
                 `-> zskiplistNode-pointer returned to the user

[zuiderkwast]

Yeah, I just meant if we want to access fields in the struct simply like node->score, then the variable size stuff needs to be in the end.

This is a balance between simplicity and performance. There is a limit. At some point, we should not add very much complexity for only a small performance improvement.

[zuiderkwast]

75% of the nodes have only one level. 75% of the remaining nodes have two levels, and so on. Only 6% of the nodes have > 2 levels. Only 1% have > 3 levels. The high-level nodes are very few, so it seems me that we don't need to focus on them.