-
Notifications
You must be signed in to change notification settings - Fork 201
Add per-context memory accounting and limits #1145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Introduce memory usage tracking at the JSContext level, allowing embedding runtimes to enforce hard memory caps per tenant or sandbox. - JSContext gains: * size_t mem_limit: configurable hard limit (0 = unlimited) * size_t mem_used: live allocation counter - New API: void JS_SetContextMemoryLimit(JSContext *ctx, size_t limit); void JS_ResetContextMemory(JSContext *ctx); size_t JS_GetContextMemoryUsage(JSContext *ctx); With this change, embedders can harden multi-tenant runtimes by sandboxing memory usage per JSContext, instead of only at the runtime level.
Can't this be accomplished with the custom memory allocation functions already? |
@saghul we can set limits at the runtime level, not per context. The runtime loses track of the context after the high-level allocation functions. |
What use case do you have which has multiple contexts per runtime? |
@saghul ultra dense use cases like serverless functions: e.g 128 contexts per runtime per core. Many companies using QuickJS face this challenge; I've seen several discussions on the official channels, and I’m dealing with the same use case. Because they don't have this capability they instantiate one runtime per context. |
There's a significant inconsistency in the allocation functions: sometimes they take rt, sometimes ctx, and in many cases if not the majority *_rt(...) are called even when ctx is available like *_rt(ctx->rt). We should enforce a clear rule: we must never deconstruct ctx and give rt in param but directly the ctx pointer. |
My planning:
This introduces breaking API changes, except If I include compatibility APIs, so it may end up as a fork/divergence. I completely understand if it’s not something maintainers want to integrate into quickjs-ng. |
Just a note that upstream may be planning to refactor how JSRuntime / JSContext works (see: bellard/quickjs#421 ). As such, it might be prudent to wait a bit to see what the plans are there (and that may also clear up some of your inconsistency concerns listed above?) |
@past-due They’re basically just renaming things: since there are no memory limits at the context level, they feel uneasy about instantiating a runtime, so they renamed runtime → context and context → realm. With proper memory limits on contexts (or realms), there’s no need to create a separate runtime+context for every script. You can run a single runtime with N contexts, each configured with its own memory limit. If you need strict multi-tenancy, you can still isolate by creating one runtime per script. My proposal is to avoid allocations from the Runtime entirely and always go through the Context APIs (or Realm, depending on naming). Right now the internal API is inconsistent: sometimes you allocate with a context, but free through the runtime. Instead, allocations and frees should always go through context APIs, which can delegate internally to the runtime if needed, never directly through the runtime. This approach prevents bypassing memory accounting, eliminates confusion from mixed APIs, and ensures that per-context memory limits are applied consistently. Regarding memory accounting: it can be enabled with a compile-time flag. If the flag is not used, the API still exists but will throw an error indicating that you need to recompile with memory accounting enabled to enforce limits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not dead set against the idea but I don't want to add functionality that only has a single user. I'd like to see first if more people want/need this.
As a bit of background, I run a quickjs-based multi-tenant service but I don't have a need for per-context memory tracking, a JSRuntime + JSContext works and performs well enough. I keep some around pre-initialized so there's always one ready to go.
The fact JSRuntime's atoms/class/shape tables are shared across contexts undeniably has some performance benefits but they're mostly minor though.
My back-of-the-napkin math says that on a 64 bits system at 128 contexts, there's ~6.6 MiB additional overhead in the JSRuntime + JSContext model vis-a-vis a single shared JSRuntime. Not nothing but pretty close to nothing in this day and age.
I guess if you preload your contexts with a lot of additional stuff (like WinterCG) the overhead will be bigger but we'd still be talking low dozens of MiBs.
void *ptr; | ||
ptr = js_calloc_rt(ctx->rt, count, size); | ||
size_t req_size; | ||
if (unlikely(__builtin_mul_overflow(count, size, &req_size))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use __builtin_mul_overflow, not universally supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, I only put it there as a draft.
Instead of using __builtin_mul_overflow, we can #include <stdckdint.h>.
For portability, I'll add a compile-time fallback with a pure C implementation as backup.
size_t actual = js_malloc_usable_size_rt(ctx->rt, ptr); | ||
|
||
if (unlikely(ctx->mem_limit && ctx->mem_used + actual > ctx->mem_limit)) { | ||
js_free_rt(ctx->rt, ptr); | ||
JS_ThrowOutOfMemory(ctx); | ||
return NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is arguably excessive. js_malloc_usable_size_rt (wrapper around malloc_usable_size on linux) is not necessarily very cheap. Probably not worth it to avoid going a few bytes over the limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I should only check once before the pre-allocation. Fragmentation and the actual physical size shouldn't be included in the limits. I will make the change
// upfront budget check: string object header + chars | ||
size_t approx_size = sizeof(JSString) + | ||
(size_t)max_len * (is_wide_char ? 2 : 1); | ||
|
||
if (unlikely(ctx->mem_limit && approx_size > (ctx->mem_limit - ctx->mem_used))) { | ||
JS_ThrowOutOfMemory(ctx); | ||
return NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better handled by breaking out js_alloc_string_rt into two separate functions, one that calls js_malloc_rt and then calls the second function to initialize the string. Call said second function from here with memory allocated with js_malloc.
@bnoordhuis Glad you took the time to review this and share your experience. I opened this PR to share the idea and start a discussion. From what I've seen, several companies are running multi-tenant architectures with a single runtime and multiple contexts, but without any per-context memory limits. The proposal here is to simplify and improve the allocate/free function signatures and introduce optional per-context memory limits, enabled at build time via a flag + some disabled features. We don't necessarily have to merge this feature into quickjs-ng, it's a proposal |
Introduce memory usage tracking at the JSContext level, allowing embedding runtimes to enforce hard memory caps per tenant or sandbox.
JSContext gains:
New API:
With this change, embedders can harden multi-tenant runtimes by sandboxing memory usage per JSContext, instead of only at the runtime level.
TODO: