Overriding the memory allocator is needlessly complex

[saghul]

I'm experimenting with using mimalloc on txiki.js and when it came to overriding the allocator in QuickJS it seems like a lot of bolierplate is needed:

static void *js_def_malloc(JSMallocState *s, size_t size)
{
    void *ptr;

    /* Do not allocate zero bytes: behavior is platform dependent */
    assert(size != 0);

    if (unlikely(s->malloc_size + size > s->malloc_limit))
        return NULL;

    ptr = malloc(size);
    if (!ptr)
        return NULL;

    s->malloc_count++;
    s->malloc_size += js__malloc_usable_size(ptr) + MALLOC_OVERHEAD;
    return ptr;
}

static void js_def_free(JSMallocState *s, void *ptr)
{
    if (!ptr)
        return;

    s->malloc_count--;
    s->malloc_size -= js__malloc_usable_size(ptr) + MALLOC_OVERHEAD;
    free(ptr);
}

static void *js_def_realloc(JSMallocState *s, void *ptr, size_t size)
{
    size_t old_size;

    if (!ptr) {
        if (size == 0)
            return NULL;
        return js_def_malloc(s, size);
    }
    old_size = js__malloc_usable_size(ptr);
    if (size == 0) {
        s->malloc_count--;
        s->malloc_size -= old_size + MALLOC_OVERHEAD;
        free(ptr);
        return NULL;
    }
    if (s->malloc_size + size - old_size > s->malloc_limit)
        return NULL;

    ptr = realloc(ptr, size);
    if (!ptr)
        return NULL;

    s->malloc_size += js__malloc_usable_size(ptr) - old_size;
    return ptr;
}

This is because the allocator customization works as follows:

typedef struct JSMallocFunctions {
    void *(*js_malloc)(JSMallocState *s, size_t size);
    void (*js_free)(JSMallocState *s, void *ptr);
    void *(*js_realloc)(JSMallocState *s, void *ptr, size_t size);
    size_t (*js_malloc_usable_size)(const void *ptr);
} JSMallocFunctions;

So if someone just wants to use mi_malloc and friends they need to copy the whole code to change a couple of lines.

Here is a proposal: alter JSMallocFunctions to take the system allocator into account:

typedef struct JSMallocFunctions {
    void *(*js_malloc)(JSMallocState *s, size_t size);
    void (*js_free)(JSMallocState *s, void *ptr);
    void *(*js_realloc)(JSMallocState *s, void *ptr, size_t size);
    size_t (*js_malloc_usable_size)(const void *ptr);
    void *(*sys_malloc)(size_t size);
    void (*sys_free)(void *ptr);
    void *(*sys_realloc)(void *ptr, size_t size);
} JSMallocFunctions;

This way, if all you want is to modify the system allocator to use mimalloc (for example), you'd do:

static const JSMallocFunctions mi_mf = {
    NULL,
    NULL,
    NULL,
    mi_malloc_usable_size,
    mi_malloc,
    mi_free,
    mi_realloc
};

Internally QuickJS would use the default functions and delegate to the overriden system allocator.

Thoughts?

PS: Why is QuickJS doing that extra MALLOC_OVERHEAD thing?

[bnoordhuis]

PS: Why is QuickJS doing that extra MALLOC_OVERHEAD thing?

It tries to compute memory usage as precisely as possible. MALLOC_OVERHEAD is libc's per-allocation overhead. Bookkeeping, basically.

Having said that, I'm not sure if MALLOC_OVERHEAD is actually accurate for glibc or musl.

[saghul]

Having said that, I'm not sure if MALLOC_OVERHEAD is actually accurate for glibc or musl.

Is there a way to find out, at runtime?

[bnoordhuis]

I don't think so. I suppose MALLOC_OVERHEAD=8 is okay as a best guess, even though for musl it can be anywhere from 0 to a few dozen bytes, depending. I'm not even going to attempt to explain what glibc does; it's an eldritch horror. :-)

[saghul]

I don't think so. I suppose MALLOC_OVERHEAD=8 is okay as a best guess, even though for musl it can be anywhere from 0 to a few dozen bytes, depending. I'm not even going to attempt to explain what glibc does; it's an eldritch horror. :-)

😅 alright then! Now back to my proposal, WDYT?

[bnoordhuis]

I don't have a strong opinion but seems fine to me.

[saghul]

@chqrlie any thoughts?

[chqrlie]

This is an interesting proposal. Here are some small problems:

• the prototype for js_usable_size is fixed and might not match that of the allocation system replacement as was experimented with various platforms already, requiring a wrapper in most cases, but that should not be an big issue.
• using a pointer for the system malloc, free and realloc handlers adds some overhead that may be measurable. Can you test with the v8 benchmarks ?
• using malloc via a pointer instead of a function call prevents some simplistic code instrumentation using macros. Again this is not a big issues as our malloc is instrumented already and more advanced tools are used for testing (MSAN, Valgrind...)

A separate path should be explored too: using memory pools for fixed size objects and favoring a set of fixed sizes for builtin objects that use flexible arrays such as JSString, JSShape and all reallocatable vectors.

[saghul]

• the prototype for js_usable_size is fixed and might not match that of the allocation system replacement as was experimented with various platforms already, requiring a wrapper in most cases, but that should not be an big issue.

Alternative allocators such as mimalloc do provide one, so in my example above I was overriding that too.

• using a pointer for the system malloc, free and realloc handlers adds some overhead that may be measurable. Can you test with the v8 benchmarks ?

I will do that indeed. Right now I wanted to get a pulse on the approach. If it turns out to perform poorly we can ditch it no problem.

[saghul]

I took a closer look at this and well... it's not that simple. The allocators just get the state, not the mf struct so it looks like an annoying change to make for little gain.

Dropping this for now.