Repository: valkey-io/valkey
File: src/defrag.c (C)

Code before:
    /* Fun fact (and a bug since forever): The key is passed to
     * moduleDefragValue as an sds string, but the parameter is declared to be
     * an robj and it's passed as such to the module type defrag callbacks.
     * Nobody can ever have used this, i.e. accessed the key name in the defrag
     * or free_effort module type callbacks. */
    void *sds_key_passed_as_robj = objectGetKey(obj);
    if (!moduleDefragValue(sds_key_passed_as_robj, obj, db->id)) defragLater(obj);
}

/* for each key we scan in the main dict, this function will attempt to defrag
 * all the various pointers it has. */
static void defragKey(defragKeysCtx *ctx, robj **elemref) {
    serverDb *db = server.db[ctx->dbid];
    int slot = ctx->kvstate.slot;
    robj *newob, *ob;
    unsigned char *newzl;
    ob = *elemref;

    /* Try to defrag robj and/or string value. */
    if ((newob = activeDefragStringOb(ob))) {
        *elemref = newob;
        if (objectGetExpire(newob) >= 0) {
            /* Replace the pointer in the expire table without accessing the old
             * pointer. */
            hashtable *expires_ht = kvstoreGetHashtable(db->expires, slot);
            serverAssert(hashtableReplaceReallocatedEntry(expires_ht, ob, newob));
        }
        if (newob->type == OBJ_HASH && hashTypeHasVolatileFields(newob)) {
            /* Check if this is a hash object containing volatile fields.
             * and update keys_with_volatile_items after defrag. */
            hashtable *keys_with_volatile_items_ht = kvstoreGetHashtable(db->keys_with_volatile_items, slot);
            serverAssert(hashtableReplaceReallocatedEntry(keys_with_volatile_items_ht, ob, newob));
        }
        ob = newob;
    }

    if (ob->type == OBJ_STRING) {
        /* Already handled in activeDefragStringOb. */
    } else if (ob->type == OBJ_LIST) {
        if (ob->encoding == OBJ_ENCODING_QUICKLIST) {
            defragQuicklist(ob);
        } else if (ob->encoding == OBJ_ENCODING_LISTPACK) {
            if ((newzl = activeDefragAlloc(ob->ptr))) ob->ptr = newzl;
        } else {
            serverPanic("Unknown list encoding");
        }
    } else if (ob->type == OBJ_SET) {
        if (ob->encoding == OBJ_ENCODING_HASHTABLE) {
            defragSet(ob);
        } else if (ob->encoding == OBJ_ENCODING_INTSET || ob->encoding == OBJ_ENCODING_LISTPACK) {
            void *newptr, *ptr = ob->ptr;

Review comment (bug):
Nit.

I wanted to avoid doing stuff with side-effects inside asserts, because if the code is ever compiled without asserts, this action will not happen. Now, we don't do that with serverAssert but we do for debugServerAssert and the generic assert in C. It's more of a good practice to not do important stuff within asserts.

Code after:
    /* Fun fact (and a bug since forever): The key is passed to
     * moduleDefragValue as an sds string, but the parameter is declared to be
     * an robj and it's passed as such to the module type defrag callbacks.
     * Nobody can ever have used this, i.e. accessed the key name in the defrag
     * or free_effort module type callbacks. */
    void *sds_key_passed_as_robj = objectGetKey(obj);
    if (!moduleDefragValue(sds_key_passed_as_robj, obj, db->id)) defragLater(obj);
}

/* for each key we scan in the main dict, this function will attempt to defrag
 * all the various pointers it has. */
static void defragKey(defragKeysCtx *ctx, robj **elemref) {
    serverDb *db = server.db[ctx->dbid];
    int slot = ctx->kvstate.slot;
    robj *newob, *ob;
    unsigned char *newzl;
    ob = *elemref;

    /* Try to defrag robj and/or string value. */
    if ((newob = activeDefragStringOb(ob))) {
        *elemref = newob;
        if (objectGetExpire(newob) >= 0) {
            /* Replace the pointer in the expire table without accessing the old
             * pointer. */
            hashtable *expires_ht = kvstoreGetHashtable(db->expires, slot);
            bool replaced = hashtableReplaceReallocatedEntry(expires_ht, ob, newob);
            serverAssert(replaced);
        }
        if (newob->type == OBJ_HASH && hashTypeHasVolatileFields(newob)) {
            /* Check if this is a hash object containing volatile fields.
             * and update keys_with_volatile_items after defrag. */
            hashtable *keys_with_volatile_items_ht = kvstoreGetHashtable(db->keys_with_volatile_items, slot);
            bool replaced = hashtableReplaceReallocatedEntry(keys_with_volatile_items_ht, ob, newob);
            serverAssert(replaced);
        }
        ob = newob;
    }

    if (ob->type == OBJ_STRING) {
        /* Already handled in activeDefragStringOb. */
    } else if (ob->type == OBJ_LIST) {
        if (ob->encoding == OBJ_ENCODING_QUICKLIST) {
            defragQuicklist(ob);
        } else if (ob->encoding == OBJ_ENCODING_LISTPACK) {
            if ((newzl = activeDefragAlloc(ob->ptr))) ob->ptr = newzl;
        } else {
            serverPanic("Unknown list encoding");
        }
    } else if (ob->type == OBJ_SET) {
        if (ob->encoding == OBJ_ENCODING_HASHTABLE) {
            defragSet(ob);