Re: [FFmpeg-devel] [PATCH 1/2] lavu/hashtable: create generic robin hood hash table

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Connor Worley:
> Signed-off-by: Connor Worley <connorbworley@gmail.com>
> ---
>  libavutil/Makefile          |   2 +
>  libavutil/hashtable.c       | 172 ++++++++++++++++++++++++++++++++++++
>  libavutil/hashtable.h       |  62 +++++++++++++
>  libavutil/tests/hashtable.c | 104 ++++++++++++++++++++++
>  4 files changed, 340 insertions(+)
>  create mode 100644 libavutil/hashtable.c
>  create mode 100644 libavutil/hashtable.h
>  create mode 100644 libavutil/tests/hashtable.c
> 
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index e7709b97d0..be75d464fc 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -138,6 +138,7 @@ OBJS = adler32.o                                                        \
>         fixed_dsp.o                                                      \
>         frame.o                                                          \
>         hash.o                                                           \
> +       hashtable.o                                                      \
>         hdr_dynamic_metadata.o                                           \
>         hdr_dynamic_vivid_metadata.o                                     \
>         hmac.o                                                           \
> @@ -251,6 +252,7 @@ TESTPROGS = adler32                                                     \
>              file                                                        \
>              fifo                                                        \
>              hash                                                        \
> +            hashtable                                                   \
>              hmac                                                        \
>              hwdevice                                                    \
>              integer                                                     \
> diff --git a/libavutil/hashtable.c b/libavutil/hashtable.c
> new file mode 100644
> index 0000000000..fb0f0aae99
> --- /dev/null
> +++ b/libavutil/hashtable.c
> @@ -0,0 +1,172 @@
> +/*
> + * Generic hashtable
> + * Copyright (C) 2024 Connor Worley <connorbworley@gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdint.h>
> +
> +#include "error.h"
> +#include "mem.h"
> +#include "hashtable.h"
> +#include "string.h"

We don't have a string.h header; the string.h from the C stdlib should
of course be included via <>.

> +
> +#define ENTRY_OCC(entry)           (entry)
> +#define ENTRY_KEY(entry)           (ENTRY_OCC(entry) + 1)
> +#define ENTRY_VAL(entry)           (ENTRY_KEY(entry) + ctx->key_size)
> +#define ENTRY_PSL(entry) (size_t*) (ENTRY_VAL(entry) + ctx->val_size)

Unaligned access. You need to align entry_size to the required alignment
of size_t and put this at the start of it the entry to avoid this.
Apart from that: The layout of your entries implies that only two
members are at a fixed offset from the start, so that accessing val and
psl requires actual additions. If you make it psl, occ, key, val, then
only val has a variable offset. This should be beneficial given that it
is the less accessed of key and val (in particular if you combine
several memcpy calls as I outline below).

(This is of course a good reason not to make the layout of the entries
public.)

> +
> +#define KEYS_EQUAL(k1, k2) !memcmp(k1, k2, ctx->key_size)
> +
> +int av_hashtable_init(AVHashtableContext* ctx, size_t key_size, size_t val_size, size_t max_entries)
> +{
> +    ctx->key_size = key_size;
> +    ctx->val_size = val_size;
> +    ctx->entry_size = 1 + key_size + val_size + sizeof(size_t);
> +    ctx->max_entries = max_entries;
> +    ctx->utilization = 0;
> +    ctx->crc = av_crc_get_table(AV_CRC_32_IEEE);
> +    if (!ctx->crc)
> +        return AVERROR_BUG;
> +    ctx->table = av_mallocz(ctx->entry_size * max_entries);

av_calloc()

> +    if (!ctx->table)
> +        return AVERROR(ENOMEM);
> +    ctx->set_key = av_malloc(key_size);
> +    if (!ctx->set_key)
> +        return AVERROR(ENOMEM);
> +    ctx->set_val = av_malloc(key_size);

Should be val_size. Your test should test scenarios with key_size !=
val_size to uncover this.

> +    if (!ctx->set_val)
> +        return AVERROR(ENOMEM);
> +    ctx->tmp_key = av_malloc(key_size);
> +    if (!ctx->tmp_key)
> +        return AVERROR(ENOMEM);
> +    ctx->tmp_val = av_malloc(val_size);
> +    if (!ctx->tmp_val)
> +        return AVERROR(ENOMEM);
> +    return 0;
> +}

(There are btw issues with what happens on error here, but we can ignore
this in the first round.)

> +
> +static size_t hash_key(const AVHashtableContext* ctx, const void* key)
> +{
> +    return av_crc(ctx->crc, 0, key, ctx->key_size) % ctx->max_entries;
> +}
> +
> +int av_hashtable_get(const AVHashtableContext* ctx, const void* key, void* val)
> +{
> +    size_t hash = hash_key(ctx, key);
> +
> +    if (!ctx->utilization)
> +        return 0;
> +
> +    for (size_t psl = 0; psl < ctx->max_entries; psl++) {
> +        size_t wrapped_index = (hash + psl) % ctx->max_entries;
> +        uint8_t *entry = ctx->table + wrapped_index * ctx->entry_size;
> +        if (!*ENTRY_OCC(entry) || *ENTRY_PSL(entry) < psl)
> +            return 0;
> +        if (KEYS_EQUAL(ENTRY_KEY(entry), key)) {
> +            memcpy(val, ENTRY_VAL(entry), ctx->val_size);
> +            return 1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +int av_hashtable_set(AVHashtableContext* ctx, const void* key, const void* val)
> +{
> +    int swapping = 0;
> +    size_t psl = 0;
> +    size_t hash = hash_key(ctx, key);
> +
> +    memcpy(ctx->set_key, key, ctx->key_size);
> +    memcpy(ctx->set_val, val, ctx->val_size);
> +
> +    for (size_t i = 0; i < ctx->max_entries; i++) {
> +        size_t wrapped_index = (hash + i) % ctx->max_entries;
> +        uint8_t *entry = ctx->table + wrapped_index * ctx->entry_size;
> +        if (!*ENTRY_OCC(entry) || (!swapping && KEYS_EQUAL(ENTRY_KEY(entry), ctx->set_key))) {
> +            if (!*ENTRY_OCC(entry))
> +                ctx->utilization++;
> +            *ENTRY_OCC(entry) = 1;
> +            memcpy(ENTRY_KEY(entry), ctx->set_key, ctx->key_size);
> +            memcpy(ENTRY_VAL(entry), ctx->set_val, ctx->val_size);
> +            *ENTRY_PSL(entry) = psl;
> +            return 1;
> +        }
> +        if (*ENTRY_PSL(entry) < psl) {
> +            if (ctx->utilization == ctx->max_entries)
> +                return 0;
> +            swapping = 1;
> +            memcpy(ctx->tmp_key, ctx->set_key, ctx->key_size);
> +            memcpy(ctx->tmp_val, ctx->set_val, ctx->val_size);

Given that set_key/val are overwritten below, these two can be done via
pointerswaps.

> +            memcpy(ctx->set_key, ENTRY_KEY(entry), ctx->key_size);
> +            memcpy(ctx->set_val, ENTRY_VAL(entry), ctx->val_size);
> +            memcpy(ENTRY_KEY(entry), ctx->tmp_key, ctx->key_size);
> +            memcpy(ENTRY_VAL(entry), ctx->tmp_val, ctx->val_size);
> +            FFSWAP(size_t, psl, *ENTRY_PSL(entry));

If you allocated foo_key and foo_val jointly, you can combine these
memcpy's. In fact, you can allocate all four them jointly with a
key-val-key-val layout and only keep one pointer in the context. You
would then use an uint8_t *set, *val pointers on the stack of this
function that are initialized at the beginning of this function.

> +        }
> +        psl++;
> +    }
> +    return 0;
> +}
> +
> +int av_hashtable_delete(AVHashtableContext* ctx, const void* key)
> +{
> +    uint8_t *next_entry;
> +    size_t hash = hash_key(ctx, key);
> +
> +    if (!ctx->utilization)
> +        return 0;
> +
> +    for (size_t psl = 0; psl < ctx->max_entries; psl++) {
> +        size_t wrapped_index = (hash + psl) % ctx->max_entries;
> +        uint8_t *entry = ctx->table + wrapped_index * ctx->entry_size;
> +        if (!*ENTRY_OCC(entry) || *ENTRY_PSL(entry) < psl)
> +            return 0;
> +        if (KEYS_EQUAL(ENTRY_KEY(entry), key)) {
> +            *ENTRY_OCC(entry) = 0;
> +            for (psl++; psl < ctx->max_entries; psl++) {
> +                wrapped_index = (hash + psl) % ctx->max_entries;
> +                next_entry = ctx->table + wrapped_index * ctx->entry_size;
> +                if (!*ENTRY_OCC(next_entry) || !*ENTRY_PSL(next_entry)) {
> +                    ctx->utilization--;
> +                    return 1;
> +                }
> +                FFSWAP(uint8_t, *ENTRY_OCC(entry), *ENTRY_OCC(next_entry));
> +                memcpy(ENTRY_KEY(entry), ENTRY_KEY(next_entry), ctx->key_size);
> +                memcpy(ENTRY_VAL(entry), ENTRY_VAL(next_entry), ctx->val_size);

These two memcpy can be combined (if one lays out key and val
sequentially in entry. Can even be combined with occ if you change the
entry layout as I outlined above.

> +                *ENTRY_PSL(entry) = *ENTRY_PSL(next_entry) - 1;
> +                entry = next_entry;
> +            }
> +        }
> +    };
> +    return 0;
> +}
> +
> +void av_hashtable_clear(AVHashtableContext* ctx)
> +{
> +    memset(ctx->table, 0, ctx->entry_size * ctx->max_entries);
> +}
> +
> +void av_hashtable_destroy(AVHashtableContext* ctx)
> +{
> +    av_freep(&ctx->table);
> +    av_freep(&ctx->set_key);
> +    av_freep(&ctx->set_val);
> +    av_freep(&ctx->tmp_key);
> +    av_freep(&ctx->tmp_val);
> +}
> \ No newline at end of file
> diff --git a/libavutil/hashtable.h b/libavutil/hashtable.h
> new file mode 100644
> index 0000000000..aac31ee922
> --- /dev/null
> +++ b/libavutil/hashtable.h
> @@ -0,0 +1,62 @@
> +/*
> + * Generic hashtable
> + * Copyright (C) 2024 Connor Worley <connorbworley@gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVUTIL_HASHTABLE_H
> +#define AVUTIL_HASHTABLE_H
> +
> +#include <stddef.h>
> +
> +#include "crc.h"
> +
> +/* Implements a hash table using Robin Hood open addressing.
> + * See: https://cs.uwaterloo.ca/research/tr/1986/CS-86-14.pdf
> + *
> + * Entry slots are layed out in memory as follows:
> + * - [1 byte] occupancy flag that indicates whether a slot if occupied.
> + * - [key_size bytes] entry key.
> + * - [val_size bytes] entry value.
> + * - [sizeof(size_t) bytes] PSL (probe sequence length), used by the Robin Hood
> + *   algorithm.

Such things should not be made public. Even if
sizeof(AVHashtableContext) were part of the ABI.

> + */
> +
> +typedef struct AVHashtableContext {
> +    size_t key_size;
> +    size_t val_size;
> +    size_t entry_size;
> +    size_t max_entries;
> +    size_t utilization;
> +    const AVCRC *crc;

You don't need the complete type here, so by using struct AVCRC *crc you
can avoid the crc.h inclusion.

> +    uint8_t *table;
> +    /* used for swaps during insertion */
> +    uint8_t *set_key;
> +    uint8_t *set_val;
> +    uint8_t *tmp_key;
> +    uint8_t *tmp_val;
> +} AVHashtableContext;

In patch 2/2 you are using this in a way that makes
sizeof(AVHashtableContext) part of the ABI. Is this intended?
Even if it is, the user should be forbidden from accessing anything in it.

> +
> +int av_hashtable_init(AVHashtableContext* ctx, size_t key_size, size_t val_size, size_t max_entries);

We generally use

> +int av_hashtable_get(const AVHashtableContext* ctx, const void* key, void* val);
> +int av_hashtable_set(AVHashtableContext* ctx, const void* key, const void* val);
> +int av_hashtable_delete(AVHashtableContext* ctx, const void* key);
> +void av_hashtable_clear(AVHashtableContext* ctx);
> +void av_hashtable_destroy(AVHashtableContext* ctx);
> +
> +#endif
> \ No newline at end of file
> diff --git a/libavutil/tests/hashtable.c b/libavutil/tests/hashtable.c
> new file mode 100644
> index 0000000000..1cdbd7afd3
> --- /dev/null
> +++ b/libavutil/tests/hashtable.c
> @@ -0,0 +1,104 @@
> +/*
> + * Generic hashtable tests
> + * Copyright (C) 2024 Connor Worley <connorbworley@gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/avassert.h"
> +#include "libavutil/hashtable.h"
> +
> +int main(void)
> +{
> +    AVHashtableContext ctx;
> +    int k, v;
> +
> +    // hashtable can store up to 3 int->int entries
> +    av_assert0(!av_hashtable_init(&ctx, sizeof(int), sizeof(int), 3));
> +
> +    // unsuccessful deletes return 0
> +    k = 1;
> +    av_assert0(!av_hashtable_delete(&ctx, &k));
> +
> +    // unsuccessful gets return 0
> +    k = 1;
> +    av_assert0(!av_hashtable_get(&ctx, &k, &v));
> +
> +    // successful sets returns 1
> +    k = 1;
> +    v = 1;
> +    av_assert0(av_hashtable_set(&ctx, &k, &v));
> +
> +    // get should now contain 1
> +    k = 1;
> +    v = 0;
> +    av_assert0(av_hashtable_get(&ctx, &k, &v));
> +    av_assert0(v == 1);
> +
> +    // updating sets should return 1
> +    k = 1;
> +    v = 2;
> +    av_assert0(av_hashtable_set(&ctx, &k, &v));
> +
> +    // get should now contain 2
> +    k = 1;
> +    v = 0;
> +    av_assert0(av_hashtable_get(&ctx, &k, &v));
> +    av_assert0(v == 2);
> +
> +    // fill the table
> +    k = 2;
> +    v = 2;
> +    av_assert0(av_hashtable_set(&ctx, &k, &v));
> +    k = 3;
> +    v = 3;
> +    av_assert0(av_hashtable_set(&ctx, &k, &v));
> +
> +    // inserting sets on a full table should return 0
> +    k = 4;
> +    v = 4;
> +    av_assert0(!av_hashtable_set(&ctx, &k, &v));
> +
> +    // updating sets on a full table should return 1
> +    k = 1;
> +    v = 4;
> +    av_assert0(av_hashtable_set(&ctx, &k, &v));
> +    v = 0;
> +    av_assert0(av_hashtable_get(&ctx, &k, &v));
> +    av_assert0(v == 4);
> +
> +    // successful deletes should return 1
> +    k = 1;
> +    av_assert0(av_hashtable_delete(&ctx, &k));
> +
> +    // get should now return 0
> +    av_assert0(!av_hashtable_get(&ctx, &k, &v));
> +
> +    // sanity check remaining keys
> +    k = 2;
> +    v = 0;
> +    av_assert0(av_hashtable_get(&ctx, &k, &v));
> +    av_assert0(v == 2);
> +    k = 3;
> +    v = 0;
> +    av_assert0(av_hashtable_get(&ctx, &k, &v));
> +    av_assert0(v == 3);
> +
> +    av_hashtable_destroy(&ctx);
> +
> +    return 0;
> +}
> \ No newline at end of file

You should add these newlines, they are AFAIK required by the spec.


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".