only initalize native ffi code on ffi import#487
Conversation
2007a9b to
a41bdab
Compare
src/ffi.c
Outdated
|
|
||
| void tjs__mod_ffi_init(JSContext *ctx, JSValue ns) { | ||
| JSValue ffiobj = JS_NewObject(ctx); | ||
| static JSValue ffiobj; |
There was a problem hiding this comment.
Note we allow threads, by way of Workers. This is not safe.
There was a problem hiding this comment.
Oh ok, wasn't aware of that.
Given that I am not sure about the JS_NewClass(ID) thread safety and thread interaction, so a thread local variable for ffiobj probably wouldn't be enough. Having a mutex here also seems to be bit overkill. Maybe implementing such a solution later on when there are more places where it is useful (e.g. lws) might be preferable.
There was a problem hiding this comment.
Class IDs are thread safe, I fixed that in QuickJS-ng by making that field part of the runtime rather than a global.
|
OK, I saw the whole file now, I had only seen the diff on my phone, which made it look weird in a way. I think the simple route here is to just attach that initializer function to the core and then use it to initialize the global state. Since the core is not a public API we can rely on that thing only ever being called once, so you're good in that case. A Worker is a whole new runtime (JSRuntime + JSContext) so it would be initialized there too, if used, but there is no problem in doing that, I think. |
a41bdab to
0be86d0
Compare
saghul
left a comment
There was a problem hiding this comment.
Nice! Can you rebase pl? I updated QuickJS and had to regenerate the bundles.
0be86d0 to
bf88ec5
Compare
bf88ec5 to
1304265
Compare
I noticed ffi init was always executed on tjs startup. This is only necessary when actually being used / being imported.
Initially I planned adding some init functions in builtins.c, but since this currently only is useful for ffi, I took the small route.