Closed Bug 1356680 Opened 4 years ago Closed 4 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

This is needed for bug 1338217 so that the patching is valid even when code is shared between modules in different threads.
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?
(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.
(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.
Taking
Assignee: nobody → luke
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)
Attachment #8859783 - Flags: review?(bbouvier)
Priority: -- → P1
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?
Attachment #8859783 - Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f5fc5a1dbc
Baldr: make builtin thunks JSRuntime-independent (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/a5f5fc5a1dbc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1359726
You need to log in before you can comment on or make changes to this bug.