1356680 - Baldr: change wasm::Runtime and thunks to be process-wide

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Luke Wagner [:luke]]

This is needed for bug 1338217 so that the patching is valid even when code is shared between modules in different threads.

Comment 1

[Luke Wagner [:luke]]

I think this is pretty easy: we put wasm::Runtime (maybe renamed to wasm::Builtins, along with WasmRuntime.h to WasmBuiltins.h, etc) in an global ExclusiveData<>.  This implies not using cx->runtime->jitRuntime->execAlloc() but rather using jit::AllocateExecutableMemory directly.  Since this function requires rounding up to 64k, and since all builtin thunks surely fit in 64k, I was thinking we could just create all the thunks all at once the first time any thunk is needed.  WDYT Benjamin?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #1)
> I think this is pretty easy: we put wasm::Runtime (maybe renamed to
> wasm::Builtins, along with WasmRuntime.h to WasmBuiltins.h, etc) in an
> global ExclusiveData<>.  This implies not using
> cx->runtime->jitRuntime->execAlloc() but rather using
> jit::AllocateExecutableMemory directly.  Since this function requires
> rounding up to 64k, and since all builtin thunks surely fit in 64k, I was
> thinking we could just create all the thunks all at once the first time any
> thunk is needed.  WDYT Benjamin?

That plan seems easily doable. There's just one caveat if the process-wide variable is global: we'd need to ensure the DeallocateExecutableMemory happens before the global ProcessExecutableMemory is destroyed (because there's an assert in its dtor that all code pages must have been deallocated).

I am willing to do this patch, but I won't be able to until next week.

Comment 3

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> There's just one caveat if the process-wide
> variable is global: we'd need to ensure the DeallocateExecutableMemory
> happens before the global ProcessExecutableMemory is destroyed (because
> there's an assert in its dtor that all code pages must have been
> deallocated).

Great point!  Probably an explicit destroy() from JS_ShutDown() is necessary then.

Comment 4

[Luke Wagner [:luke]]

Taking

Comment 5

[Luke Wagner [:luke]]

Attachment: process-wide-builtins

The patch ended up being a bit more invasive because the natural way to express things, once all allocation was done up-front and per-process, was rather different.

A summary of changes:
 - there is now a single BuiltinThunks struct stored in a global variable
 - initialization is done lazily using a lock to synchronize init
 - there is only one contiguous allocation, stored in BuiltinThunks and released by BuiltinThunks
 - there is no need for the intermediate BuiltinThunk; just an array of CodeRange that are all relative to the one code allocation
 - rather than map from func* to func*, the need to enumerate everything up front suggested separate schemes for SymbolicAddress vs. InlinableNative: SymbolicAddress's thunks are just stored in a simple array (indexed by SymbolicAddress) and the inlineable natives are stored in a map with key (InlinableNative, signature)

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8859783 [details] [diff] [review]
process-wide-builtins

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

Code motion, renamings and logic changes: this is hard to say it's 100% clear there's no bug, but the high-level logic looks good.

::: js/src/wasm/WasmBuiltins.cpp
@@ +711,5 @@
> +using TypedNativeToFuncPtrMap =
> +    HashMap<TypedNative, void*, TypedNative, SystemAllocPolicy>;
> +
> +
> +

nit: 2 extra lines to remove here

@@ +756,5 @@
> +//
> +// Thunks are created for two kinds of C++ callees, enumerated above:
> +//  - SymbolicAddress: for statically compiled calls in the wasm module
> +//  - Imported JS builtins: optimized calls to imports
> +// 

nit: trailing space

@@ +854,5 @@
> +        return false;
> +
> +    size_t allocSize = AlignBytes(masm.bytesNeeded(), ExecutableCodePageSize);
> +
> +    thunks->codeSize = AlignBytes(masm.bytesNeeded(), ExecutableCodePageSize);

That's allocSize.

@@ +936,3 @@
>  {
> +    MOZ_ASSERT(builtinThunks);
> +    const BuiltinThunks& thunks = *builtinThunks;

Could you move the definition of thunks closer to its first use?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f5fc5a1dbc
Baldr: make builtin thunks JSRuntime-independent (r=bbouvier)

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/a5f5fc5a1dbc