1358396 - Factor wasm::DebugState out from wasm::Code

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

(Spun off from bug 1338217)

We want wasm::Code to be shareable, so the per-instance mutable debug state needs to be factored out and hung off the instance instead.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1358396-debug-state.patch

This introduces wasm/WasmDebug.{cpp,h} and a new type, DebugState.  DebugState hangs off the wasm instance now.  All clients that previously went through the Code for this functionality now go directly to the DebugState object.

For this to work, the DebugState must sometimes access the Code, so the Code is owned by a RefPtr rather than a UniquePtr.  This will change again when bug 1338217 is complete.

I would appreciate an extra look on the memory management involved in creating the Code and the DebugState, in Module::instantiate, just to ensure that it is correct.

The patch is otherwise straight renaming / refactoring.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1358396-debug-state-v2.patch

Minor update to the patch: make the debug field of Instance 'const'.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8860362 [details] [diff] [review]
bug1358396-debug-state-v2.patch

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

Great change; thanks for splitting this out into a separate patch.

::: js/src/wasm/WasmDebug.cpp
@@ +26,5 @@
> +#include "vm/Debugger.h"
> +#include "vm/StringBuffer.h"
> +
> +#include "wasm/WasmBinaryToText.h"
> +#include "wasm/WasmValidate.h"

I'd run 'make check-style'; I think "mozilla/BinarySearch" needs to go before the jit/ includes (with \n before and after) and ds/Sort.h belongs up with the rest.  Also a nit: no need for \n between ds/ jit/, vm/ and wasm/ #includes.  Also, before pushing I'd make sure to do a non-unified build (setting FILES_PER_UNIFIED_FILE = 1 in moz.build).

::: js/src/wasm/WasmDebug.h
@@ +79,5 @@
> +
> +class DebugState
> +{
> +    MutableCode code_;
> +    const SharedMetadata     metadata_;

nit: can you column-align 'code_' with the rest?

::: js/src/wasm/WasmModule.cpp
@@ +903,5 @@
>      auto globalSegment = GlobalSegment::create(linkData_.globalDataLength);
>      if (!globalSegment)
>          return false;
>  
> +    auto code = MutableCode(js_new<Code>(Move(codeSegment), *metadata_, maybeBytecode));

nit: how about `Mutable code(js_new...)`?

@@ +909,5 @@
>          return false;
>  
> +    // The debug object must be present even when debugging is not enabled: It
> +    // provides the lazily created source text for the program, even if that
> +    // text is a placeholder message when debugging is not enabled.

So does that mean, in the final state after bug 1338217, when debugging is not enabled, each Instance will get a DebugState (holding this mutable source text state) even though the Code is shared?  If so, that sounds great; maybe over time DebugState will have other state for managing best-effort-when-debug-mode-not-enabled-but-debugging-anyways support.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1358396-debug-state-v3.patch

Patch ready to land, with nits addressed, carrying luke's r+.  Blocked on bug 1356680.

Comment 5

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09c6b4abfed352655fbfe9d18c8def4af4c7516f
Bug 1358396 - Refactor wasm debug state into its own object. r=luke

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09c6b4abfed3