Closed Bug 1237508 Opened 5 years ago Closed 5 years ago

Odin: make AsmJSModule derive wasm::Module

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(2 files)

MozReview Request: Bug 1237508 - Odin: remove function index from Export (r?bbouvier)
58 bytes, text/x-review-board-request
bbouvier
: review+
Details
MozReview Request: Bug 1237508 - Odin: make AsmJSModule derive wasm::Module (r?bbouvier)
58 bytes, text/x-review-board-request
bbouvier
: review+
Details 
While working on some patches in bug 1234985 I realized it'd be a lot simpler if AsmJSModule could derive wasm::Module.  This patch set ended up removing a fair amount of boilerplate some things.
https://reviewboard.mozilla.org/r/29845/#review26659

This patch isn't essential, but I happened to notice that Export::funcIndex was only used by ModuleGenerator and so it could be moved out of Export.
https://reviewboard.mozilla.org/r/29847/#review26657

This is patch is primarily code motion:
 \- Just like with ModuleGenerator/Module, AsmJSModule is made into a read-only object that is created in MV::finish() and all the construction is put inside MV.
 \- To avoid having to pipe all the constructor args of Module through AsmJSModule's constructor, I created a ModuleData struct which holds the raw contents and a UniqueModuleData is passed instead. This ended up allowing a bunch of code removal since now ModuleGenerator could store a UniqueModuleData and simply hand that over.  This allowed so much boilerplate removal that I did the same for AsmJSModule.  Overall this patch is 200 lines smaller.
 \- Virtual functions ended up being a pretty natural way to express certain patterns where AsmJSModule and wasm::Module have to cooperate like trace, addSizeOfMisc, and destruction, but mostly everything stays non-virtual.

This patch will enable the export-object construction to be moved into WasmModule.cpp so that it can be shared with Wasm.cpp in bug 1234985.  The key is that there is only 1 object (WasmModuleObject) that always points to the wasm::Module (which may be the base of an AsmJSModule).

::: js/public/Utility.h:469
(Diff revision 1)
> +    {}

Note that this is just copying the analogous code from mfbt/UniquePtr.h's DefaultDelete.  Without this, you can't assign a JS::UniquePtr<T, JS::DeletePolicy<T>> to a JS::UniquePtr<U, JS::DeletePolicy<U>>, even if T\* is convertible to U\* since UniquePtr also requires DeletePolicy<T> to be convertible to DeletePolicy<U>.
Comment on attachment 8704973 [details]
MozReview Request: Bug 1237508 - Odin: remove function index from Export (r?bbouvier)

https://reviewboard.mozilla.org/r/29845/#review26715

Nice!
Attachment #8704973 - Flags: review?(bbouvier) → review+
Attachment #8704974 - Flags: review?(bbouvier) → review+
Comment on attachment 8704974 [details]
MozReview Request: Bug 1237508 - Odin: make AsmJSModule derive wasm::Module (r?bbouvier)

https://reviewboard.mozilla.org/r/29847/#review26717

Looks really nice! I much like where this is going, in particular the new mutable-during-construction then immutable transitions this introduces, in addition to the ones already there. The biggest comment is about hiding specifics of where fields are (data_ or data_->pod), but that can be done in a subsequent patch, when the interfaces get more stable.

::: js/src/asmjs/AsmJS.h:22
(Diff revision 1)
>  #include "vm/NativeObject.h"

Do you still need vm/NativeObject.h now?

::: js/src/asmjs/AsmJS.cpp:70
(Diff revision 1)
> +TraceNameField(JSTracer* trc, PropertyName* const* name, const char* label)

So this is a (non const) pointer to a const pointer to a PropertyName, right? (in this case, I'd personally put first star next to the const keyword, and would put a space before the second star, to explicit the fact that the const is relative to the first pointer, but I have no idea what the guideline style is when it comes to pointer to const pointers).

::: js/src/asmjs/AsmJS.cpp:411
(Diff revision 1)
> +    const UniqueConstAsmJSModuleData data_;

This name is a bit too generic: how about parentData_ or moduleData_ or commonData_? or just module_?

::: js/src/asmjs/AsmJS.cpp:1687
(Diff revision 1)
> +    {

style nit: {} or { }, no need for a blank line

::: js/src/asmjs/AsmJS.cpp:2205
(Diff revision 1)
> +        auto mutedErrors = MutedErrorsBool(parser_.ss->mutedErrors());

Can't it just be

MutedErrorsBool mutedErrors(parser_.ss->mutedErrors());

?

::: js/src/asmjs/AsmJS.cpp:2222
(Diff revision 1)
> +

nit: two blank lines

::: js/src/asmjs/AsmJS.cpp:4030
(Diff revision 1)
>      // Atomic accesses may be made on shared integer arrays only.

pre-existing, but there are no more shared typed arrays per se, so this comment might need an update.

::: js/src/asmjs/AsmJS.cpp:7762
(Diff revision 1)
> -        Rooted<AsmJSModuleObject*> clone(cx, NewAsmJSModuleObject(cx));
> +        if (!moduleObj->module().asAsmJS().clone(cx, &moduleObj))

The fact that moduleObj is switched in this line is not obvious at first sight (i was about to suggest to have a local variable for moduleObj->module().asAsmJS(), then realized my mistake), maybe add a comment here?

::: js/src/asmjs/WasmModule.h:348
(Diff revision 1)
> +// ModuleData holds the trivially-memcpy()able serializable portion of

nit: ModuleCacheablePod

::: js/src/asmjs/WasmModule.h:566
(Diff revision 1)
> -    // See WASM_DECLARE_SERIALIZABLE.
> +    // This function reports the code/data memory usage of this module.

While the style of this file suggests to have a blank line after a comment, it's weird to have a single-line comment followed by a blank line. Not a big deal though, so it's up to you.

::: js/src/asmjs/WasmModule.cpp:79
(Diff revision 1)
> +    MOZ_ASSERT(bytes_ != 0);

Would this happen, in case of OOM?

::: js/src/asmjs/WasmModule.cpp:751
(Diff revision 1)
>      // Patch callsites and returns to execute profiling prologues/epililogues.

nit: pre-existing, epi**li**logues.

::: js/src/asmjs/WasmModule.cpp:900
(Diff revision 1)
> -    size_t upperBound = codeRanges_.length();
> +    size_t upperBound = data_->codeRanges.length();

Thinking about it, there is a general issue of knowing where a specific field is: is it in data\_? in data\_->pod? It feels like implementing and using accessors everywhere would hide where the data is located, in a lot of different places, for a lot of fields (just skimming the functions around here: callSites, codeRanges, heapAccesses, functionBytes, etc.). That would be great.

This r+ grants an unlimited right to implement such accessors in this patch. Of course, this could be done in another patch/bug later, when things get more stable wrt to our wasm implementation.
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> > +TraceNameField(JSTracer* trc, PropertyName* const* name, const char* label)

I'll add a 'typedef PropertyName* PropertyNamePtr' so I can write 'const PropertyNamePtr*' :)

> This name is a bit too generic: how about parentData_ or moduleData_ or
> commonData_? or just module_?

module_ sounds good; I'll change all the moduleData's to that too.

> ::: js/src/asmjs/AsmJS.cpp:2205
> (Diff revision 1)
> > +        auto mutedErrors = MutedErrorsBool(parser_.ss->mutedErrors());
> 
> Can't it just be
> 
> MutedErrorsBool mutedErrors(parser_.ss->mutedErrors());

I thought so too, but apparently that doesn't count as an explicit cast.

> The fact that moduleObj is switched in this line is not obvious at first
> sight (i was about to suggest to have a local variable for
> moduleObj->module().asAsmJS(), then realized my mistake), maybe add a
> comment here?

Good point.  Moving the decl of 'module' up above and making it a pointer (so it can be assigned inside the dynamicallyLinked() branch) makes it much more clear.

> While the style of this file suggests to have a blank line after a comment,
> it's weird to have a single-line comment followed by a blank line. Not a big
> deal though, so it's up to you.

Good point.  Ooh, I can move it up to be under 'trace' so all 3 vfuns are grouped together (and they all have SM-wide idiomatic names so require no comment).

> ::: js/src/asmjs/WasmModule.cpp:79
> (Diff revision 1)
> > +    MOZ_ASSERT(bytes_ != 0);
> 
> Would this happen, in case of OOM?

Good question but no, because UniquePtr() only calls the deleter if the ptr is != nullptr.

> Thinking about it, there is a general issue of knowing where a specific
> field is: is it in data\_? in data\_->pod?

That's a good point, and I was wondering this as well.  I feel like member_ vs. data_->member is meaningful (the former means "this data dies with the MV/MG", the latter means "this lives on with the module").  But data_->member vs. data_->pod.member is less meaningful and that bugged me.  Ooh, I could have ModuleData derive ModuleCacheablePod and similarly for AsmJSModuleData.
https://hg.mozilla.org/mozilla-central/rev/de03cd950775
https://hg.mozilla.org/mozilla-central/rev/4be634e52d65
https://hg.mozilla.org/mozilla-central/rev/b354cf7abcb4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.