1186424 - Odin: Refactor and move files to prepare the parallelization

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

So I think there's gonna be a lot of patches, so I prefer moving the patches here, to avoid spamming everybody.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.move.things.patch

So this is the original patch from the other bug, before any comments. I think it's easier to have it on top, as it's really only moving things around (and prevents some wild rebasing with the other patches). Plus, when everything is clean, I can just move back the big Module class to the AsmJSValidate file.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.dont.depend.mc.global.emitting.patch

This eliminates the dependency of the Emitter to ModuleCompiler::Global, as suggested in your last comment in the other bug.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.mc.global.static.ctors.patch

This adds the static constructors to ModuleCompiler::Global, preventing the need for the friendship betwen Global and ModuleCompiler (and cleaning some code, thanks to helpers).

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.rename.mc.mv.patch

s/ModuleCompiler/ModuleValidator

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

The next step should be to take one little thing which belongs to the ModuleValidator (I was thinking about the stackOverflowLabel for instance), and move it into the new ModuleCompiler class. This would help identify what are the needed changes, and I suppose a lot of other things will need to move as well, just to be able to compile.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8637288 [details] [diff] [review]
1.move.things.patch

From IRL discussion, we've established a different strategy that would keep ModuleValidator/ModuleCompiler in the separate .cpp's and keep a lot less in AsmGlobals.h.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8637290 [details] [diff] [review]
2.dont.depend.mc.global.emitting.patch

Feel free to re-flag for review if any of these were orthogonal and can be landed earlier.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Make Globals be used only during validation

This is what patch 2 from the previous series did. As it's actually independent from the big refactoring bits, it is ready for review.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Split ModuleCompiler into ModuleValidator/ModuleCompiler

So when implementing it, I figured out that ModuleValidator (MV) has to outlive ModuleCompiler (MC), because of the function pointer tables, which are checked once all the functions have been compiled. As a matter of fact, ModuleGlobals (MG) doesn't need its own lifo, we can just use MV's one everywhere.

This means MC and MV share some lifetime. In particular, the AsmJSModule has to be shared among the two, and it's a bit unfortunate / ugly. Any thought about it?

Note this patch doesn't embed lifo's memory pointers in the IR, the next patch does it.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Try to make MC standalone

We still need the ModuleGlobals structure, because of two things:

1) MC needs to be able to peek into the Func (ordered) array functions_, when generating Entries. Namely, exported functions contain the Func's index in this array.
2) MC needs to patch the FuncPtrTables at the end of the compilation (that happens in MC::finish()).

So even if we embed Func and FuncPtrTable and FFI's information directly into the IR (including a pointer to the signature), we can't get rid of the ModuleGlobals yet. Any thoughts about this?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Try to make MC standalone

Now with updated comments in the code.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4. Take MC out of CheckFunction

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5. Another attempt at splitting the files

With this split in mind, the main thread would block on all functions being compiled on helper threads, before doing the final phases (FinishModule) on the main thread. With the baseline compiler, it will need to proceed in another way, though: the baseline compilation should start as soon as the start off thread jobs are launched, so the final phases would have to be done by the helper thread doing the codegen. Note that in the current state, it would need some bouncing back and forth between the main thread and codegen helper thread: after functions are compiled, the module still needs to check the functions pointers tables, and that all used functions are actually defined...

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8639371 [details] [diff] [review]
1. Make Globals be used only during validation

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

Nice!

Comment 15

[Luke Wagner [:luke]]

Looking good!  It'd be great to see if there are any other isolated patches that can be hoisted out above the big code motion patches.

A few questions:
 - Could AsmFunction be unified with ModuleGlobals::Func?
 - While ModuleCompiler is currently LifoAlloc'd to avoid putting ModuleCompiler in AsmCompile.h, in the next bug, when ModuleCompiler executes on its own thread, could ModuleCompiler be on the stack of that thread and most of the functions removed from AsmCompile.h?

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> Note that in the current state,
> it would need some bouncing back and forth between the main thread and
> codegen helper thread: after functions are compiled, the module still needs
> to check the functions pointers tables, and that all used functions are
> actually defined...

I think it's fine if the main (validator) thread ends up blocking on the codegen helper thread to complete.  This join could return the MacroAssembler used by MC as well as a Vector<Label*> for all the functions so that the validator thread could do GenerateStubs/FinishModule (which I was hoping would address issues in comment 10).  Also, the use of .entry().bound() in CheckAllFunctionsDefined() could just as well check ModuleCompiler::Func::defined_.

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> In particular, the AsmJSModule has
> to be shared among the two, and it's a bit unfortunate / ugly. Any thought
> about it?

So I was thinking about how this would work, in particular about being careful not to race on AsmJSModule since it's mutated by both validation and emitting.  Here's my thought:
 - when analyzing the global section of the module, before MC is created, AsmJSModule can be owned by MV
 - when the MC is created, MV can hand it over to the MC (and null out its own ScopedPtr)
 - the MC calls AsmJSModule functions like addCodeRange etc during codegen
 - when MV joins on MC, it gets the AsmJSModule back which it can then use to 'finish()' etc
The one exception is that MV wants to call AsmJSModule::tryRequireHeapLengthToBeAtLeast() while validating function bodies.  For this, I think you can move this logic from AsmJSModule to MV and instead pass it to the AsmJSModule during finish().  MC currently calls AsmJSModule::minHeapLength() to pass to MIRGenerator::initMinAsmJSHeapLength(), but this is for an optimization that has never helped, so I'd just delete it entirely (from MIRGenerator and jit/RangeAnalysis.cpp).

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5. Embed AsmFunction into MV::Func

(In reply to Luke Wagner (PTO) [:luke] from comment #15)
> A few questions:
>  - Could AsmFunction be unified with ModuleGlobals::Func?

Not really: the AsmFunction and Func have very different lifetimes. In particular, an AsmFunction has to be created before the entire validation, while the Func is created once it's validated. If we were to merge the AsmFunction and the Func, then the Func would need to be created before the entire validation, and the Func contains a Signature, which can't have a default ctor.

What we can do, instead, is to embed the AsmFunction into the Func. It's cleaner, wrt passing in/out arguments in several functions.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6. ModuleCompileData

(In reply to Luke Wagner (PTO) [:luke] from comment #15)
>  - While ModuleCompiler is currently LifoAlloc'd to avoid putting
> ModuleCompiler in AsmCompile.h, in the next bug, when ModuleCompiler
> executes on its own thread, could ModuleCompiler be on the stack of that
> thread and most of the functions removed from AsmCompile.h?

I think so. With this patch, MC can be created at the very last minute.

> I think it's fine if the main (validator) thread ends up blocking on the
> codegen helper thread to complete.  This join could return the
> MacroAssembler used by MC as well as a Vector<Label*> for all the functions
> so that the validator thread could do GenerateStubs/FinishModule (which I
> was hoping would address issues in comment 10).  Also, the use of
> .entry().bound() in CheckAllFunctionsDefined() could just as well check
> ModuleCompiler::Func::defined_.
> 

So we can indeed finish getting rid of the ModuleGlobals in the MC, by introducing another data structure, ModuleCompileData, that's passed to the MC, and taken over by the MV after MC is done. Just passing the MacroAssembler (and other data) wasn't doable because it can't be moved easily. I didn't need a Vector<Label*> though, so I am not sure you were thinking at the same thing here.

MCD has a clear lifetime and clear ownership, and can be simply transmitted through message passing.


> (In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> > In particular, the AsmJSModule has
> > to be shared among the two, and it's a bit unfortunate / ugly. Any thought
> > about it?
> 
> So I was thinking about how this would work, in particular about being
> careful not to race on AsmJSModule since it's mutated by both validation and
> emitting.  Here's my thought:
>  - when analyzing the global section of the module, before MC is created,
> AsmJSModule can be owned by MV
>  - when the MC is created, MV can hand it over to the MC (and null out its
> own ScopedPtr)
>  - the MC calls AsmJSModule functions like addCodeRange etc during codegen
>  - when MV joins on MC, it gets the AsmJSModule back which it can then use
> to 'finish()' etc

I may be missing something, but the AsmJSModule is constantly read during the validation (checking for global names, etc.). How would nulling out the ptr work then? Should we copy the useful AsmJSModule's data directly inside the MV, instead?

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8641162 [details] [diff] [review]
5. Embed AsmFunction into MV::Func

Could MV::Func just be created earlier so its lifetime coincides with AsmFunction?

Comment 19

[Luke Wagner [:luke]]

Oh wait, I'm being silly, the whole point of MV::Func is to be local to validation while AsmFunction is the thing produced by validation and consumed by compilation.  n/m

Comment 20

[Luke Wagner [:luke]]

Oh wait, I was reading the patch talking about "MV::Func" but my question was about AsmGlobals::Func which does seem like it'd have the same lifetime as AsmFunction (if you created AsmGlobals::Func earlier).

Comment 21

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> I may be missing something, but the AsmJSModule is constantly read during
> the validation (checking for global names, etc.). How would nulling out the
> ptr work then? Should we copy the useful AsmJSModule's data directly inside
> the MV, instead?

I think it's mostly just those 3 arg names (globalArgumentName(), importArgumentName(), bufferArgumentName()) which could be copied into the MV.  I think the rest of the global state is already in the MV.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 8641166 [details] [diff] [review]
6. ModuleCompileData

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

Sounds great!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1604,5 @@
> +typedef Vector<Func*> FuncVector;
> +
> +} // namespace
> +
> +class ModuleCompileData

"Data" is pretty vague; how about ModuleCompileResults?

Comment 23

[Luke Wagner [:luke]]

So thinking about this AsmJSModule question again, it seems like, with baseline compilation and background Ion compilation, the ModuleValidator should maintain exclusive ownership of the AsmJSModule throughout validation and any information that the ModuleCompiler needs should be communicated through the bytecode stream or ModuleGlobals.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Make Globals be used only during validation

rebased, carrying over r+

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Split ModuleCompiler into ModuleValidator/ModuleCompiler

Even though this patch doesn't reach the final ideal state we'd like, I'd like to take an incremental approach here and have it reviewed, to prevent long rebasing over trivial changes, if you don't mind. Requesting review on all the following patches, including this one.

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Try to make MC standalone

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4. Take MC out of CheckFunction

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5. Embed AsmFunction into MV::Func

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6. ModuleCompileResults

rebased + renaming

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7.wip.ownership.patch

A WIP implementing ideas from the latest comments, very much started (the heap requirements parts haven't be done yet).

Comment 31

[Luke Wagner [:luke]]

Comment on attachment 8648137 [details] [diff] [review]
2. Split ModuleCompiler into ModuleValidator/ModuleCompiler

OK.

Comment 32

[Benjamin Bouvier [:bbouvier] (inactive)]

Optimistically landing. A try run showed a trivial static analysis issue (ModuleCompileResults' ctor wasn't explicit) and a trivial GC analysis issue (ModuleValidator had to be special-cased during rooting analysis, as it's a stack class).

Comment 33

[Benjamin Bouvier [:bbouvier] (inactive)]

Marking as [leave-open] as everything hasn't been done yet.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc3a4689c88
https://hg.mozilla.org/integration/mozilla-inbound/rev/65b067184f10
https://hg.mozilla.org/integration/mozilla-inbound/rev/e41895c02c0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdeb7135e541
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c7d36f29ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/4612c09141ae

Comment 35

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f8dd6bbde6ce for ASan mochitest-1 leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=12914438&repo=mozilla-inbound

Comment 36

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7. Remove min heap length optimization

So, eventually, I figured out that the best way to rebase was to locally backout the other patches that touched AsmJSValidate during the week, rebase my changes, and apply the inverted backed out patches. Meh, versioning control systems.

This patch removes the min heap length optimization, and can be applied atop the other ones in this bug. I am planning to post a fully folded patch quickly, but this could be interesting to have a look at, for an easier review of the big patch coming ahead.

Comment 37

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 8. Remove AsmJSModule from the ModuleCompiler

Not too hard. We have to temporarily store more things in the module compile results, that are handed over to the AsmJSModule when the MV takes ownership of the ModuleCompileResults (code ranges, script counts, perf profiling data).

Comment 38

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 9. Fix memory issue wrt AsmJSNumLit

(not tested on tbpl yet, but this should fix the issue that caused the backout)

Comment 39

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 10. Remove MV::Func's dependency in ModuleCompiler thread's functions

Comment 40

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Folded patch (from 1 to 10)

Luke, here's the folded patch from the 10 previous patches.

Among the new things, in patches 7 up to 10:
- the min heap length optimization is removed in the mirgenerator and in range analysis. You said it didn't bring us a lot, plus it was a security hole (we had some off-by-one issues at some point there). So it sounds like a win (I'll benchmark a few things before going on with parallelization).
- Part 8: removes the AsmJSModule from the ModuleCompiler; see the attached comment 37.
- Part 9: it should fix the memory issue that caused the backout, but I need to check. The fix is to lifo alloc AsmFunction's vectors members (rather than using the default system policy), as the AsmFunction is now lifo-alloc'ed. I wonder if we could have automation to detect this kind of issues (that is: a lifo-alloc'ed structure has one or more members that are not PODs, free memory in their dtor and aren't lifo-alloc'ed) at compile time, or during static analysis.
- With part 10, splitting the files should be made easier (I haven't tried yet). This introduces a new structure, ModuleCompileResults::Func, which is based off a ModuleValidator::Func (and points directly to its members). It's the input to the MIR generation thread (contains bytecode, etc.). It is alloc'ed in the thread's lifo, because it shouldn't survive longer than the compiling thread. Its members can be updated by the mirgen/codegen thread, but it's the only updater, otherwise everything should be read-only. Is that safe?

Also, do you want me to try splitting the patch again? You told me on vidyo-call that folding was fine, but it really is a big patch again...

Comment 41

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8652950 [details] [diff] [review]
Folded patch (from 1 to 10)

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +6614,5 @@
> +    ModuleValidator::Func* callee;
> +    if (!CheckFunctionSignature(f.m(), callNode, Move(signature), calleeName, &callee))
> +        return false;
> +
> +    f.patchPtr(entryAt, (uint8_t*) &callee->entry());

Regarding my previous question about the sharedness of entries, that's the only place where it's (almost) used: the validator thread is expected to write the address of the entry here, while the other thread may change the state of the entry. When compilation is over (after we've returned back the ModuleCompileResults), the validator thread finishes generating the stubs and mutates the entry.

So we have:
- during validation/compilation: validator thread is reader, compiler thread writes it
- after compilation, when generating stubs: validator thread is the only writer (there are no more compiler threads).

To make this clearer, I've created "Label const *const readOnlyEntry()" function, but it still looks like a Bad API (anybody could misuse it). If you have any other idea, I'd gladly hear it!

Comment 42

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Folded patch (from 1 to 10)

Switching from feedback to review, as I'm now pretty confident in what I've done.

The diff:
- Good news, everyone! Some code didn't have to actually move, so I've put it back at its initial position, and the patch's size reduced by 9 KB. Yay!
- Renamed ModuleCompileResults::Func into ModuleCompileFunc. Was hesitating with "ModuleCompileInput", for symmetry with ModuleCompileResults (which could be renamed "ModuleCompileOutput", for perfect symmetry, but it doesn't much tell what it actually contains). Really, if MC and MV were to live in the same file, this could just be ModuleCompiler::Func. Any other name suggestions would be appreciated.
- Some cosmetic changes.
- Added the super-const ModuleValidator::readOnlyEntry() function, as mentioned in the self-review comment.

Will post to try as soon as the trees reopen.

Comment 43

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Addendum patch: use index to reference function's entries on the main thread, and allocate their labels on the compiler thread

What we've discussed yesterday. Now, Q&A.

Q: why not using a simple Vector<Label>?
A: because the MAsmJSCall nodes expect a Label*, and as the Vector gets resized, the address of the Label can change => kaboum.

Q: solution?
A: use a Vector<Label*>, lazily allocate the Labels on the compiler thread thanks to js_new, to make sure their location doesn't change over time.

Q: js_new is slow. Have you considered alternatives?
A: yes:
- use the lifo alloc from the AsmJSModule, but these Label allocations happen on the compiler thread, while the lifo lives in the main thread, and I don't have the certainty that the lifo alloc is even reentrant, and that sounds scary.
- use the lifo alloc from the FunctionCompiler, but the Label outlives the FC's lifetime.
- create the entry's Label in the MV, when creating the MV::Func; provide this label *and* the funcIndex to the ModuleCompileFunc (which is the input of the compiler thread MIRgen). The issue is concurrency, as in this case, if a function calls another function that hasn't been parsed yet, then the another function doesn't have a label yet, and we can't allocate it on the compiler thread.
- do another indirection and use an index rather than the label in the MAsmJSCall. That has another concurrency issue, as it would force us to block on the entire compilation before doing function's codegen, and that's bad.

Thanks to jandem for discussing alternatives on irc, the final conclusion was that one malloc per function shouldn't be too much, hopefully. Thoughts?

Comment 44

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Folded patch

Skipping the review flag, waiting for your feedback on the previous item.

Comment 45

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Remove heap length optimization

Good news, everyone! The patch for removing the min heap optimization can actually be split from the rest of the patch quite easily.

Comment 46

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Changes to AsmJSValidate

Other patches folded.

Comment 47

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #43)
What about giving ModuleComileResults a LifoAlloc and, when the MC hands over the MCR to the MV, the MV stores in a ScopedPtr so they live until the MV is destroyed?

Comment 48

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Addendum patch: use index to reference function's entries on the main thread, and allocate their labels on the compiler thread

(In reply to Luke Wagner [:luke] from comment #47)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #43)
> What about giving ModuleComileResults a LifoAlloc and, when the MC hands
> over the MCR to the MV, the MV stores in a ScopedPtr so they live until the
> MV is destroyed?

Thanks for the idea, I'll try that. Just in case it doesn't work (but it should, really), my patch plus some fixed memory leaks for dead functions and compilation failures.

Comment 49

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Changes to AsmJSValidate

Here we go

Comment 50

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Changes to AsmJSValidate

Fixing your second comment on irc: indeed, we don't need to leak the function's index in the AsmJSModule::Function, as the corresponding MV::Func can be looked up by name. That had *not* to be the case at some point during the work done in this bug, at some point in a far, far away past.

Comment 51

[Luke Wagner [:luke]]

Comment on attachment 8654200 [details] [diff] [review]
2. Changes to AsmJSValidate

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

Great!  This design really makes sense; thanks for working to get here.  With the small changes below (which I'd like to scan once over) we'll be done.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1186,5 @@
> +class ModuleCompileFunc
> +{
> +    PropertyName* name_;
> +    const AsmFunction* bytecode_;
> +    const VarTypeVector& argTypes_;

Storing a reference might be ok now, but not once this stuff outlives the MV (background Ion compilation) so perhaps better to store by value now (with LifoAllocPolicy).

@@ +1370,5 @@
> +        bool defined() const { return defined_; }
> +        uint32_t funcIndex() const { return funcIndex_; }
> +
> +        Label& entry() const { return *entry_; }
> +        void setEntry(Label* entry) { entry_ = entry; }

I think you could remove entry_ and, since the Vector<Label*> will be kept alive until the end of MV, use compileResults->functionEntries[func->funcIndex()].  (Memory savings not a big deal, but just deduplication of state.)

@@ +1425,5 @@
> +        const Func& elem(unsigned i) const { return *elems_[i]; }
> +    };
> +
> +    typedef Vector<FuncPtrTable*> FuncPtrTableVector;
> +    typedef Vector<Func*> FuncVector;

Pre-existing, but could you move this up to above FuncPtrVector and rename FuncPtrVector to ConstFuncVector?

@@ +1643,5 @@
> +
> +    FuncVector                        functions_;
> +    FuncPtrTableVector                funcPtrTables_;
> +
> +    GlobalMap                         globals_;

I'd remove intervening \n.

@@ +1661,5 @@
> +
> +    bool                              canValidateChangeHeap_;
> +    bool                              hasChangeHeap_;
> +
> +    bool                              supportsSimd_;

I'd remove intervening \n.

@@ +2012,5 @@
> +        Signature* signature = moduleLifo_.new_<Signature>(Move(sig));
> +        if (!signature)
> +            return false;
> +        *lifoSig = signature;
> +        ExitDescriptor exitDescriptor(name, signature);

Pre-existing, but this code LifoAlloc's a new Signature every time, even if it's already in exits_.  You need a sig for the exitDescriptor used to do the lookup, but this could be |&sig|.  On match, |*lifoSig = p->key().sig()|, and then on miss you would lifo-allocate the sig, create a new ExitDescriptor with the new sig (perhaps call the original one 'lookup' and call the new one 'key').

@@ +2163,5 @@
> +        return false;
> +    }
> +
> +    // End-of-compilation utils
> +    bool takeCompileResultsOwnership(ModuleCompileResults* compileResults) {

There is an implicit assumption the caller keeps compileResults alive longer than MV.  Looking at the caller, *technically* this assumption is violated (MCR is destroyed before MV and so, e.g., is trash in ~MV).  It could be trivially fixed by moving the MCR decl above the MV decl.  In the thready future, though, this will lead to mild (performance, not correctness) false-sharing (compiler thread accessing validator thread's stack).  How about decoupling these issues so that MC js_new's a MCR and stores it in a ScopedJSDeletePtr member variable.  Then have methods:
  void ModuleCompiler::finish(ScopedJSDeletePtr<ModuleCompileResults>*) // going out
  void ModuleValidator::finishFunctionBodies(ScopedJSDeletePtr<ModuleCompileResults>*)  // coming in
(Where finishFunctionBodies is this function merged with finishFunctionBodies, since they both happen together.) ModuleValidator would store the MCR as a ScopedJSDeletePtr. Can you also put the ModuleCompiler stack object inside CheckFunctions(Parallel|Sequential) and have:
  bool CheckFunctions(ModuleValidator&, ScopedJSDeletePtr<ModuleCompileResults>*)
This is all pretty symmetric to ModuleValidator::finish / CheckModule().

@@ +2170,5 @@
> +        // Move function's entries labels
> +        MOZ_ASSERT(compileResults->numFunctionEntries() == numFunctions());
> +        for (size_t i = 0; i < numFunctions(); i++) {
> +            Func& func = function(i);
> +            unsigned funcIndex = func.funcIndex();

Based on above comments, I think this loop can be deleted but, if not: isn't i == funcIndex?

@@ +2180,5 @@
> +        for (size_t i = 0; i < compileResults->numCodeRanges(); ++i) {
> +            AsmJSModule::CodeRange& codeRange = compileResults->codeRange(i);
> +            MOZ_ASSERT(codeRange.isFunction());
> +            unsigned nameIndex = codeRange.functionNameIndex();
> +            if (!module().addFunctionCodeRange(compileResults->name(nameIndex), Move(codeRange)))

Something really subtle is happening here: in the compiler thread a vector of names are being built in MCR::names_ and the index is being put in CodeRange::nameIndex_ which indexes AsmJSModule::names_ which happens to be built in such a way that the same index works.  How about instead having a MCR::FunctionCodeRange which derives AsmJSMOdule::CodeRange and adds a PropertyName* field (and sets nameIndex_ to UINT32_MAX).  MCR::codeRanges_ would be a Vector<FunctionCodeRange>.  Then MC::finishGeneratingFunction creates a MCR::FunctionCodeRange with the given name (so you can remove MCR::names_).  Finally, here, you pass AsmJSModule::addFunctionCodeRange(codeRange.name(), codeRange) and AsmJSModule::addFunctionCodeRange will assert nameIndex_ == UINT32_MAX and then initialize it.

@@ +2282,5 @@
> +    }
> +
> +    void startFunctionBodies() {
> +        module_->startFunctionBodies();
> +    }

After finishFunctionBodies() is merged with takeCompileResultsOwnership (as proposed above), could you move startFunctionBodies() right above it?

@@ +2296,5 @@
> +    }
> +
> +    void buildCompilationTimeReport(JS::AsmJSCacheResult cacheResult, ScopedJSFreePtr<char>* out) {
> +        ScopedJSFreePtr<char> slowFuns;
> +#ifndef JS_MORE_DETERMINISTIC

Prexisting: can you move slowFuns under the #ifndef?

@@ +2413,5 @@
>  class MOZ_STACK_CLASS ModuleCompiler
>  {
>    public:
> +    // Map exitIndex to the corresponding exit's Signature.
> +    typedef HashMap<unsigned, const Signature*> ExitMap;

I think this is dead.

@@ +2418,4 @@
>  
>    private:
> +    ExclusiveContext*     cx_;
> +    AsmJSParser&          parser_;

I think parser_ is also dead.

@@ +2422,5 @@
> +
> +    ModuleCompileResults* compileResults_;
> +
> +    bool                  usesSignalHandlersForInterrupt_;
> +    bool                  usesSignalHandlersForOOB_;

Perhaps wrap these up into some ModuleCompileInput (passed by const&, stored by value)?  I was thinking this set of ambient inputs might grow over time.

@@ +3165,5 @@
>  
>  // Encapsulates the building of an asm bytecode function from an asm.js function
>  // source code, packing the asm.js code into the asm bytecode form that can
>  // be decoded and compiled with a FunctionCompiler.
>  class FunctionBuilder

rs=me for followup patch to rename FunctionCompiler to FunctionValidator and then rename this to FunctionCompiler (for symmetry with ModuleValidator/Compiler).

@@ +5922,5 @@
>          }
>  
> +        unsigned globalIndex = global->varOrConstIndex();
> +        // Global data offset
> +        unsigned offsetAt = indexAt;

I'd stick with just indexAt.

@@ +6751,5 @@
> +    if (!CheckFuncPtrTableAgainstExisting(f.m(), tableNode, name, Move(sig), mask, &table))
> +        return false;
> +
> +    f.patch32(globalDataOffsetAt, table->globalDataOffset());
> +    f.patchPtr(signatureAt, (uint8_t*) &table->sig());

I see you've been careful to make sure that all the sigs here are LifoAlloc'ed and not stored in movable Vector memory.  Still, it'd be a bit safer to express this constraint (that Signatures must be LifoAlloc'd) in the type.  How about we create a 'class LifoSignature : public Signature' and then you remove replace the generic patchPtr/readPtr with patchSignature/readSignature and these take LifoSignature.  To further tighten this, LifoSignature could hide its constructors and only expose an operator new that took a LifoAlloc (forcing you to allocate via 'new (lifo) Signature`).  With this change, I think you can go back to putting some things in Vectors (e.g. FuncPtrTable) that you had to LifoAlloc b/c of the Signature (they'd store a Signature* instead).  For now, all these signatures could come out of the MV's Lifo (it outlives), but eventually we'll need a LifoAlloc that lives as long as background Ion compilation.  This will also avoid the reinterpret_cast<Signature*>(readPtr())s.

@@ +10535,5 @@
>  }
>  
> +static bool
> +GenerateMIR(ModuleCompiler& m, LifoAlloc& lifo, ModuleCompileFunc* func,
> +            MIRGenerator** mir)

This now fits on one line.

@@ +10625,5 @@
>  
>      if (func->defined())
>          return m.failName(fn, "function '%s' already defined", FunctionName(fn));
>  
> +    asmFunc->setNumLocals(f.numLocals());

Pre-existing, but if you added an AsmFunction::addFormal called by FunctionBuilder::addFormal (like addVariable), then AsmFunction could maintain this itself w/o the separate call.

@@ +10654,5 @@
>      if (!codegen)
>          return false;
>  
> +    Label* funcEntry;
> +    if (!m.ensureFunctionEntry(func.funcIndex(), &funcEntry))

How about "getFunctionEntry" (get often mean "create new or access existing" in SM)?

@@ +10657,5 @@
> +    Label* funcEntry;
> +    if (!m.ensureFunctionEntry(func.funcIndex(), &funcEntry))
> +        return false;
> +
> +    AsmJSFunctionLabels labels(*funcEntry, m.stackOverflowLabel());

Pre-existing: I just noticed that this class holds a reference and so should be a MOZ_STACK_CLASS to prevent accidental escape.

@@ +10715,5 @@
> +
> +        auto* compileFunc = lifo.new_<ModuleCompileFunc>(
> +            func->name(), func->bytecode(), func->sig().args(),
> +            func->funcIndex(), func->srcBegin(), func->compileTime()
> +        );

Could we:
 - fold ModuleCompileFunc into AsmFunction since they seem to represent the same thing and have the same lifetime
 - have FunctionBuilder set these fields (name, args, index, etc) inside some FunctionBuilder::finish() that was called inside CheckFunction() at the end?
 - remove MV::Func::bytecode_ and change CheckFunction()'s outparam to be an AsmFunction (MV::Func has a very different lifetime from AsmFunction anyhow)

@@ +12294,1 @@
>               ScopedJSDeletePtr<AsmJSModule>* module)

Pre-existing, but I assume this can fit on one line now.

Comment 52

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Changes to AsmJSValidate

Thanks for the comments, they are really appreciated! Here's an update version of the patch.

Comment 53

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Changes to AsmJSValidate

With a few things solved from our discussions on irc

Comment 54

[Luke Wagner [:luke]]

Comment on attachment 8654975 [details] [diff] [review]
2. Changes to AsmJSValidate

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

Beautiful, thanks!  Just nits and some changes that can be in follow-up bugs.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1093,5 @@
> +    unsigned funcIndex_;
> +    unsigned srcBegin_;
> +    unsigned compileTime_;
> +
> +    RetType returnedType_;

Maybe move returnedType_ up to right under argTypes_ (like a Signature) and separate Bytecode with a \n?

@@ +1660,5 @@
> +              : name_(name), sig_(sig)
> +            {}
> +        };
> +
> +        static HashNumber hash(const Lookup& d) {

I'd remove the intervening \n (logically grouping the 3 required definitions of HashPolicy).

@@ +2298,5 @@
> +
> +    void startFunctionBodies() {
> +        module_->startFunctionBodies();
> +    }
> +    bool finishFunctionBodies(ScopedJSDeletePtr<ModuleCompileResults>& compileResults) {

Since the parameter is mutated, can you pass a * instead of & (makes it clear at the callsite that "this argument is mutated").

@@ +2305,5 @@
> +
> +        // When an interrupt is triggered, all function code is mprotected and,
> +        // for sanity, stub code (particularly the interrupt stub) is not.
> +        // Protection works at page granularity, so we need to ensure that no
> +        // stub code gets into the function code pages.

I just noticed that this comment and the haltingAlign() are no longer necessary since we no longer mprotect code to implement interrupt (we SuspendThread/pthread_kill).  We might have to audit a bit of code to remove (I noticed one assert in AsmJSModule::finish), but for now could add a "TODO: this is no longer true, consider removing"?

@@ +2314,5 @@
> +
> +        // Hand in code ranges, script counts and perf profiling data to the AsmJSModule
> +        for (size_t i = 0; i < compileResults_->numCodeRanges(); ++i) {
> +            AsmJSModule::FunctionCodeRange& codeRange = compileResults_->codeRange(i);
> +            MOZ_ASSERT(codeRange.functionNameIndex() == UINT32_MAX);

addFunctionCodeRange() does this already, so I'd be fine removing since it's mostly irrelevant to this simple from-here-to-there loop.

@@ +2469,2 @@
>        : cx_(cx),
> +        tokenStream_(ts),

I think this can also be removed if the use in finishGeneratingFunction is moved to happen at the end of validation (so in MV).  When we have MC on a separate thread, this will be necessary to avoid the race condition, but I'm fine to have that be a follow-up bug.  Similarly, ExclusiveContext may be used off the main thread, but it's not thread-safe itself, so we'll probably want to remove that field too as a follow-up bug.

@@ +3447,4 @@
>      /************************************************** End of build helpers */
> +
> +    bool finish(const ModuleValidator::Func& f) {
> +        return func_.finish(f.sig().args(), f.name(), f.funcIndex(), f.srcBegin(), f.compileTime());

I requested this function but, now that I read this in the context of the other changes, there's no point in having this "helper" compared to having CheckFunction() directly call asmFun->finish(func->...) directly; sorry; could you do that?

@@ +10572,5 @@
>  }
>  
> +static bool
> +GenerateMIR(ModuleCompiler& m, LifoAlloc& lifo, AsmFunction* func,
> +            MIRGenerator** mir)

Single line.  Also, could you pass a AsmFunction&?  It could almost be a const& but for accmulateCompileTime.

@@ +10677,5 @@
> +}
> +
> +static bool
> +GenerateCode(ModuleCompiler& m, AsmFunction& func, MIRGenerator& mir,
> +             LIRGraph& lir)

Single line.

@@ +10735,5 @@
>      // function by the LifoAllocScope inside the loop.
>      LifoAlloc lifo(LIFO_ALLOC_PRIMARY_CHUNK_SIZE);
>  
> +    ModuleCompileInputs mci(m.module().usesSignalHandlersForInterrupt(),
> +                            m.module().usesSignalHandlersForOOB());

To avoid duplication, how about making a 'ModuleCompileInputs MV::compileInputs() const' member function (passed inline in the constructor call) here and in CheckFunctionsParallel?

@@ +12357,5 @@
>          return false;
>      if (*moduleOut)
>          return true;
>  
> +    ModuleValidator mv(cx, parser);

With ModuleCompiler gone, seems fine to keep this 'm'.

Comment 55

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Remove TokenStream in MC

Thanks for the review!

The TokenStream in ModuleCompiler is used for retrieving the lineno / colno from the offsets, in interrupt checks (at loops' heads or at functions' heads) and for the debug / profiling info. However, we can do smarter:
- for the profiling info, just put the lineno/colno in AsmFunction as well as the offset to the start of function (for use in failOffset)
- for the interrupt checks, introduce two new opcodes in our internal IR: Stmt::InterruptCheck (at functions' heads) and I32::InterruptCheck (at loops' head, emits an interrupt check and then just decodes an i32).

The last usage is in noteBasicBlock, which is removed in the next patch.

Comment 56

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4. Remove perf block annotations in Odin

asmjs/AsmJSLink.cpp     |   28 --------
 asmjs/AsmJSModule.cpp   |   21 ------
 asmjs/AsmJSModule.h     |   18 -----
 asmjs/AsmJSValidate.cpp |  164 ++++++++++++++----------------------------------
 jit/PerfSpewer.cpp      |   85 ------------------------
 jit/PerfSpewer.h        |   14 +---
 6 files changed, 52 insertions(+), 278 deletions(-)

And this subsumes bug 1178840, which can be closed if this patch is accepted.

Comment 57

[Luke Wagner [:luke]]

Comment on attachment 8655544 [details] [diff] [review]
3. Remove TokenStream in MC

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

Great!  Think we can make one simplification here:

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3001,5 @@
>      I32X4ExtractLane,
>  
>      // Specific to AsmJS
>      Id,
> +    InterruptCheck, /* interrupt checks at loops' heads */

I think we can get away without an int32-returning interrupt check by moving the interrupt check from the condition block to the loop body block (it doesn't exactly matter where the interrupt check).

@@ +6703,5 @@
>  
>      MOZ_ASSERT_IF(sig.retType() != RetType::Void, sig.retType() == retType);
>  
> +    uint32_t lineno = f.readU32();
> +    uint32_t column = f.readU32();

Perhaps use symmetric ReadCallLineCol?

Comment 58

[Luke Wagner [:luke]]

Comment on attachment 8655545 [details] [diff] [review]
4. Remove perf block annotations in Odin

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ -4603,5 @@
>      /*************************************************************************/
>    private:
> -    unsigned sourceOffsetFromBytecodePosition(size_t pos)
> -    {
> -        // TODO (bug 1178840) : implement me!

Looks like you can close this bug now :)

Comment 59

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd47f6229b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/775373908366

Comment 60

[Benjamin Bouvier [:bbouvier] (inactive)]

Try build returned green for the first patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6a411247321

Comment 61

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Remove tokenStream in MC - alternative version

> I think we can get away without an int32-returning interrupt check by moving the interrupt check from the condition block to the loop body block (it doesn't exactly matter where the interrupt check).

But we still need two opcodes! The interrupt check at a function's head is a statement of its own, as it can be decoded without additional effort. However, when decoding a loop, the general scheme is to decode the conditions (int32) and a single statement, which can be a statement or a block (list of statements). If we want to keep usesSignalHandlersForInterrupt out of ModuleCompiler, we need to make it transparent for the Emit function that the statement is either A) an interrupt check and a statements list, or B) just a statements list. For that purpose, we need two different opcodes: InterruptCheckHead and InterruptCheckLoop. This patch implements this.

Comment 62

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Remove TokenStream in MC

Please pick your favorite among the two...

Comment 63

[Luke Wagner [:luke]]

Comment on attachment 8655888 [details] [diff] [review]
3. Remove tokenStream in MC - alternative version

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

Great, thanks!

Comment 64

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5. Remove ExclusiveContext from MC

Last bit before ModuleCompiler can be used on another thread! I am not sure about a few things: using Maybe<> seemed to be the easiest way, even though it makes the code way uglier. Also, the ModuleCompileResults ctor initialization list relies on initialization order, which I thought was C++ UB, but it locally works somehow. Try building this changeset, happy to hear any other options.

Comment 65

[Luke Wagner [:luke]]

Comment on attachment 8656073 [details] [diff] [review]
5. Remove ExclusiveContext from MC

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1310,2 @@
>  #if defined(MOZ_VTUNE) || defined(JS_ION_PERF)
> +    Vector<AsmJSModule::ProfiledFunction, 0, JitAllocPolicy> profiledFunctions_;

Instead of switching these all to JitAllocPolicy, could these be switched to SystemAllocPolicy instead?  The only thing the default alloc policy (TempAllocPolicy) does extra is report errors on the context, but we don't need that within asm.js compilation (in general SM there is the invariant that if you 'return false' you must have reported an error, but as a special case, asm.js compilation does NoExceptionPending() at the root of the callstack.

Also, can you hoist these Vector instantiations up into typedefs with the other vectors above?

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfd47f6229b6
https://hg.mozilla.org/mozilla-central/rev/775373908366

Comment 67

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5. Remove ExclusiveContext from MC

Right! Here we go.

Comment 68

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #51)
> Comment on attachment 8654200 [details] [diff] [review]
> 2. Changes to AsmJSValidate
> 
> Review of attachment 8654200 [details] [diff] [review]:
> -----------------------------------------------------------------
> rs=me for followup patch to rename FunctionCompiler to FunctionValidator and
> then rename this to FunctionCompiler (for symmetry with
> ModuleValidator/Compiler).

Do you mean that the FunctionCompiler should be the one that's used for checking and create the bytecode, while the FunctionValidator should be the one that's used for emitting the MIR? It sounds inverted: the ModuleValidator ought to work with FunctionValidators, and the ModuleCompiler should work with FunctionCompilers, right?

Comment 69

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #68)
Oops, you're right.  I thought FunctionCompiler was doing validation, but it's not and it's correctly named.  It's only FunctionBuilder that should be renamed FunctionValidator.

Comment 70

[Luke Wagner [:luke]]

Comment on attachment 8656448 [details] [diff] [review]
5. Remove ExclusiveContext from MC

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1311,2 @@
>  #if defined(MOZ_VTUNE) || defined(JS_ION_PERF)
> +    Vector<AsmJSModule::ProfiledFunction, 0, SystemAllocPolicy> profiledFunctions_;

Can you also hoist a ProfiledFunctionVector and then align all these member variable names vertically?

@@ +1333,1 @@
>  #endif // defined(MOZ_VTUNE) || defined(JS_ION_PERF)

I'd just remove these default ctor calls (they'll be called automatically).

@@ +2400,5 @@
>  
>      ModuleCompileInputs compileInputs() const {
> +        JSCompartment* compartment = cx()->compartment();
> +        return ModuleCompileInputs(CompileCompartment::get(compartment),
> +                                   CompileRuntime::get(compartment->runtimeFromAnyThread()),

I just noticed that, if you have:
  CompileCompartment* compartment = CompileCompartment::get(cx()->compartment());
you can call compartment->runtime() to get the CompileRuntime.

@@ +3448,5 @@
>    private:
> +    typedef HashMap<uint32_t, BlockVector, DefaultHasher<uint32_t>, JitAllocPolicy> LabeledBlockMap;
> +    typedef HashMap<size_t, BlockVector, DefaultHasher<uint32_t>, JitAllocPolicy> UnlabeledBlockMap;
> +    typedef Vector<size_t, 4, JitAllocPolicy> PositionStack;
> +    typedef Vector<Type, 4, JitAllocPolicy> LocalVarTypes;

Can you turn all the rest of these Vectors (in this file and the Args in MIR.h) to SystemAllocPolicy?  Then you won't need the Maybe<> below.

@@ -10577,5 @@
>      int64_t before = PRMJ_Now();
>  
>      FunctionCompiler f(m, func, lifo);
> -    if (!f.init())
> -        return false;

Don't forget to add this back (when switching to SystemAllocPolicy).

Comment 71

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5. Remove ExclusiveContext from MC - SystemAllocPolicy

Comment 72

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6. Rename FunctionBuilder to FunctionValidator

And the renaming patch, which should be the last one in this bug.

Comment 73

[Luke Wagner [:luke]]

Comment on attachment 8656596 [details] [diff] [review]
6. Rename FunctionBuilder to FunctionValidator

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

Great!

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be54e36bdb5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ac4de45665
https://hg.mozilla.org/integration/mozilla-inbound/rev/478b111da2fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb36973dc953

Comment 75

[Benjamin Bouvier [:bbouvier] (inactive)]

A try build before the renaming and changing JitAllocPolicy to SystemAllocPolicy returned green.

Comment 76

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

https://hg.mozilla.org/mozilla-central/rev/be54e36bdb5a
https://hg.mozilla.org/mozilla-central/rev/74ac4de45665
https://hg.mozilla.org/mozilla-central/rev/478b111da2fc
https://hg.mozilla.org/mozilla-central/rev/fb36973dc953