Allow specifying allocation functions for workers

[Vizepi]

In worker_func the runtime is created using the default allocation functions (JS_NewRuntime does that)
In the same vein as js_worker_new_context_func we should have a js_worker_new_runtime_func which would allowto pass whatever allocation function we want.

Bonus point: we should also be able to provide freeing callbacks for worker's runtime and context as in the current state, we are unable to release any resource we would have allocated and stored in user_opaque for context (and runtime as well once we have a callback for allocation)

Proposed changes in quickjs-libc.c

Add a user pointer

• static void * js_worker_user_opaque = NULL;

Modify existing callback to take user opaque:

• static JSContext *(*js_worker_new_context_func)(JSRuntime *rt, void *);

Add a 3 static callbacks:

• static JSRuntime *(*js_worker_new_runtime_func)(void *);
• static void(*js_worker_free_runtime_func)(JSRuntime *, void *);
• static void(*js_worker_free_context_func)(JSContext *, void *);

Add setters for the user opaque and the new callbacks

• Adapt signature of js_std_set_worker_new_context_func to take an additional void *
• Add void js_std_set_worker_new_runtime_func(JSRuntime *(*func)(void * user_opaque))
• Add void js_std_set_worker_free_context_func(void(*func)(JSContext *ctx, void * user_opaque))
• Add void js_std_set_worker_free_runtime_func(void(*func)(JSRuntime *rt, void * user_opaque))

In worker_func:

• Replace rt = JS_NewRuntime(); by rt = js_worker_new_runtime_func(js_worker_user_opaque );
• Replace ctx = js_worker_new_context_func(rt); by ctx = js_worker_new_context_func(rt, js_worker_user_opaque);
• Replace JS_FreeContext(ctx); by js_worker_free_context_func(ctx, js_worker_user_opaque);
• Replace JS_FreeRuntime(rt); by js_worker_free_runtime_func(rt, js_worker_user_opaque);

[bnoordhuis]

That won't work, it's inherently unsound. You wouldn't expect jemalloc to be able to free memory allocated with dlmalloc, but that's basically what you're asking here.

Memory allocated in the main runtime can be freed in the worker and vice versa, but allocator A can't free memory from allocator B.

[Vizepi]

I think you did not understand the issue, or maybe there is something I missed. Currently allocations in worker thread can only be performed by js_def_* functions right? But if I have my client using a custom allocator, all these allocations are done outside their allocator and in the system's allocator instead. Is this correct?

[bnoordhuis]

Just so we're on the same page:

1. You're asking to replace JSMallocFunctions in the worker, right? That's unsound for the reasons I mentioned.

2. Currently the worker does not inherit the allocator from the main thread and that's a bug.

[bnoordhuis]

After reviewing the relevant code in quickjs-libc.c, I think there's no direct risk of mixing allocators. We sidestep that problem by using plain malloc/free inside postMessage.

Your feature request is a way to specify a custom allocator for the worker, correct? It's not out of the question but I don't really like the idea of introducing more global variables.

[bnoordhuis]

After thinking about it for a few minutes, this is acceptable:

void js_std_set_worker_new_runtime_func(JSRuntime *(*func)(void));

I don't see the point of that js_worker_user_opaque static global variable inside quickjs you suggested. It's a singleton so you might just as well store it on your side.

[Vizepi]

Yes the main issue I have is the current inability to provide custom allocator for runtime. I'm not a big fan of adding globals either, but I don't see other ways to do it. I didn't dig into the code to see what triggers the creation of a worker thread, but maybe if it comes from an other runtime we could transfer the allocator from there.
About a global opaque you're right it is just moving the problem, so I agree with you it may be preferable to keep it like it is.
And finally as long as there is no resource lifetime to manage that is associated to the newed runtime and context, there should be no need for free functions.

[bnoordhuis]

PR in #1246.