1334885 - Allow wasm signal handlers to work with multiple threads in a runtime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently the wasm signal handlers treat JS runtimes as if they were single threaded.  Most of these places can be fixed by just operating on a JSContext instead of a JSRuntime.  The main impact of this patch is that on OS X separate mach exception handler threads are created for each JSContext that runs wasm code, plus the thread that created the runtime in the first place (so that process wide signal handlers are in place for interrupting Ion code).

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8831518 [details] [diff] [review]
patch

Review of attachment 8831518 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/wasm/WasmInstance.cpp
@@ +539,5 @@
>          return false;
>  
> +    // Make sure any signal handling infrastructure required for this thread
> +    // has been installed.
> +    if (!EnsureSignalHandlers(cx))

This can be a hot path and we're going to try to add an inline JIT path for this sometime soon so could you instead move this check to the top of Module::instantiate() and update the comment in Runtime.cpp accordingly?

Also, it looks like EnsureSignalHandlers(cx) doesn't report an error, so you need to here.  It'd be better of course if EnsureSignalHandlers(cx) reported its own error, but I'm guessing that it can't because it's called during JSRuntime::init.

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +1232,4 @@
>  #ifdef JS_SIMULATOR
>          (void)ContextToPC(context);  // silence static 'unused' errors
>  
> +        void* pc = cx->runtime()->simulator()->get_pc_as<void*>();

(I'm assuming the simulator is getting moved into the cx in a later patch.)

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #1)
> ::: js/src/wasm/WasmSignalHandlers.cpp
> @@ +1232,4 @@
> >  #ifdef JS_SIMULATOR
> >          (void)ContextToPC(context);  // silence static 'unused' errors
> >  
> > +        void* pc = cx->runtime()->simulator()->get_pc_as<void*>();
> 
> (I'm assuming the simulator is getting moved into the cx in a later patch.)

Ermm, it is in JSContext after bug 1325050, I just forgot to test this patch in a simulator build.  It doesn't look like the simulator will have an impact on this patch, though.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #1)
> ::: js/src/wasm/WasmInstance.cpp
> @@ +539,5 @@
> >          return false;
> >  
> > +    // Make sure any signal handling infrastructure required for this thread
> > +    // has been installed.
> > +    if (!EnsureSignalHandlers(cx))
> 
> This can be a hot path and we're going to try to add an inline JIT path for
> this sometime soon so could you instead move this check to the top of
> Module::instantiate() and update the comment in Runtime.cpp accordingly?

Hmm, looking at Module::instantiate() it seems like it creates a WasmInstanceObject which is placed in some zone and can be used by any thread operating in that zone.  If this interpretation is right then with this change it would be possible for a thread to call into wasm without having its signal handlers installed.

Another option that might be best is to just install the signal handlers when creating any thread which can run JS (the runtime's main thread, and soon any additional cooperative threads created for it).  I don't see any downsides to this, its only effect is to add more threads on OS X and per a google search OS X will let you create thousands of threads in a process.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: followup

Diff vs. the previous patch.  Also, s/zone/compartment/ in the first sentence in comment 3.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8834959 [details] [diff] [review]
followup

Review of attachment 8834959 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2600c68923ce
Allow wasm signal handlers to work with multiple threads in a runtime, r=luke.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2600c68923ce