Skip to content
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.

Sign up for GitHub

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

QJS hooks and GC hooks #747

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+32 −2
Open

QJS hooks and GC hooks #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f73b7db
Implementing registration for hooks
Dec 10, 2024
4c60f3d
Implemented hooks for GC
Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions quickjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@ struct JSRuntime {
void *user_opaque;
void *libc_opaque;
JSRuntimeFinalizerState *finalizers;

/* Unified handler for all the engine hooks */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have something similar for finalizers, how is this generic? It only deals with GC. I'd call it GC hooks.

Alternatively we could fold finalizers in there too, but I'm not too thrilled about it.

JS_BOOL (*hooks)(JSRuntimeHooks type, void* opaque);
};

struct JSClass {
Expand Down Expand Up @@ -1440,7 +1443,17 @@ static void js_trigger_gc(JSRuntime *rt, size_t size)
(uint64_t)rt->malloc_state.malloc_size);
}
#endif
JS_RunGC(rt);
//To ensure JS_RunGC cannot be executed again within callbacks, disable and restore it after.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the style match the surroundings, here and elsewhere.

size_t tmp_threshold = rt->malloc_gc_threshold;
rt->malloc_gc_threshold=-1;

if((rt->hooks == NULL) || rt->hooks(JS_HOOK_GC_BEFORE,NULL)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for not running GC if the before handler returns FALSE? There is an API to disable GC already, what's the use case here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hook could change the threshold, but you are then re-setting it.

Copy link
Contributor Author

@KaruroChori KaruroChori Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but while functionally similar they have different applications. Setting the GC on/off in specific critical regions can get a bit tricky with async/await. Not only that, but doing so would mask that a request was done, preventing the application from saying "oh yes, I had to delay it, I am now in a safe region, let's clean up things". Ensuring the before event can say, "hey no, please don't" is a fail-safe. Ideally the custom callback could set up alternative mechanism to run that gc event, just a bit later when no critical task it taking place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds very theoretical ;-) Why can't the mechanism that knows if we are safe or not enable / disable GC while in a critical region?

Copy link
Contributor Author

@KaruroChori KaruroChori Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the application requirements:

  • Option 1: I don't care, gc events as needed and scheduled by the engine. Not good for real time applications.
  • Option 2: gc events only when I determine regions are safe (possibly never). I will trigger it myself when and if I want it. The only feasible approach for the strictest real time applications.
  • Option 3: I would prefer gc to take place based on the engine's suggestions, but I cannot do it right now. I take notice of that, and skip it for now. In the meanwhile I ask the system to move itself in a safe state when possible, schedule the gc event in that safe state, and recover the current job from where it was left.

Having the option to skip gc events while tracking them enables option 3 which is better than option 2 for a good class of real time applications.
It is not theoretical, it is literally how my application works.
It does not mean it is good design, just that at the very least it is slightly above a purely theoretical scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for elaborating! Looks like option 3 could work as follows:

  • GC event hits
  • app checks if it can afford it, if not it disabled GC and tries to move to the safe state
  • Once the safe state is hit, enable GC and run it
  • goto 1

Wouldn't that work?

Copy link
Contributor Author

@KaruroChori KaruroChori Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the time the app can check if it is ok to run a GC event, it is already in the body of before.
If before does not stop it by failing, and the caller obliges, what does?
Unless you plan to split before and evaluate in two separate hooks, but it would be virtually identical just for more complexity since there are no further places where they would be called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before can stop GC from running by disabling it.

So before get hit, but because you disable it it doesn't run. Then when you want GC the app can enable it and run it.

What wouldn't work?

JS_RunGC(rt);
if(rt->hooks != NULL)rt->hooks(JS_HOOK_GC_AFTER,NULL);
}

rt->malloc_gc_threshold=tmp_threshold;

rt->malloc_gc_threshold = rt->malloc_state.malloc_size +
(rt->malloc_state.malloc_size >> 1);
}
Expand Down Expand Up @@ -1810,6 +1823,8 @@ JSRuntime *JS_NewRuntime2(const JSMallocFunctions *mf, void *opaque)
rt->malloc_state = ms;
rt->malloc_gc_threshold = 256 * 1024;

rt->hooks = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call this gc_handlers, gc_hooks or gc_callbacks, just hooks feels wrong.


bf_context_init(&rt->bf_ctx, js_bf_realloc, rt);

init_list_head(&rt->context_list);
Expand Down Expand Up @@ -1922,7 +1937,8 @@ void JS_SetDumpFlags(JSRuntime *rt, uint64_t flags)
rt->dump_flags = flags;
}

size_t JS_GetGCThreshold(JSRuntime *rt) {
size_t JS_GetGCThreshold(JSRuntime *rt)
{
return rt->malloc_gc_threshold;
}

Expand All @@ -1932,6 +1948,11 @@ void JS_SetGCThreshold(JSRuntime *rt, size_t gc_threshold)
rt->malloc_gc_threshold = gc_threshold;
}

void JS_SetHooksHandler(JSRuntime *rt, JS_BOOL (*fn)(JSRuntimeHooks type, void* opaque))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why take an opaque if you are not doing anything with it? An opaque would make sense if you support multiple callbacks, so each could get their own.

{
rt->hooks = fn;
}

#define malloc(s) malloc_is_forbidden(s)
#define free(p) free_is_forbidden(p)
#define realloc(p,s) realloc_is_forbidden(p,s)
Expand Down
9 changes: 9 additions & 0 deletions quickjs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,13 @@ typedef void JSRuntimeFinalizer(JSRuntime *rt, void *arg);

typedef struct JSGCObjectHeader JSGCObjectHeader;

/* Engine hooks */
typedef enum JSRuntimeHooks{
JS_HOOK_NONE, //Don't use, just to catch potential bugs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this needs to exist. You want to avoid 0? Make JS_HOOK_GC_BEFORE start at 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a habit of mine. I usually keep 0 as an error value for enums, and I usually give them a mnemonic in place of starting with 1. It helps when testing debug builds where memory is initialized to 0.
In this specific context I agree it is not really useful.

JS_HOOK_GC_BEFORE, //Run before a GC event takes place. No payload. Return false if the hooks opposes the GC event to take place.
JS_HOOK_GC_AFTER, //Run after a GC event took place. No payload. Return value ignored.
} JSRuntimeHooks;

JS_EXTERN JSRuntime *JS_NewRuntime(void);
/* info lifetime must exceed that of rt */
JS_EXTERN void JS_SetRuntimeInfo(JSRuntime *rt, const char *info);
Expand All @@ -332,6 +339,8 @@ JS_EXTERN void JS_SetMemoryLimit(JSRuntime *rt, size_t limit);
JS_EXTERN void JS_SetDumpFlags(JSRuntime *rt, uint64_t flags);
JS_EXTERN size_t JS_GetGCThreshold(JSRuntime *rt);
JS_EXTERN void JS_SetGCThreshold(JSRuntime *rt, size_t gc_threshold);
/* register a hook dispatcher for this runtime. The handler should always return true for hooks which are not supported. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API suggests one could register multiple hooks, but you really cannot. The finalizers API can, on the contrary.

Now, if you add the ability to register multiple handlers, then the whole boolean return thing doesn't work...

JS_EXTERN void JS_SetHooksHandler(JSRuntime *rt, JS_BOOL (*fn)(JSRuntimeHooks type, void* opaque));
/* use 0 to disable maximum stack size check */
JS_EXTERN void JS_SetMaxStackSize(JSRuntime *rt, size_t stack_size);
/* should be called when changing thread to update the stack top value
Expand Down
Loading