Luke Wagner [:luke] Assignee
	Description • 11 years ago

One way to fix the parse-time memory spike associated with asm.js code is to not create parse nodes at all by writing an asm.js parser (bug 854061) which would also improve load time.  However, we'd probably like to fix the memory spike problem sooner than it looks like bug 854061 would be ready.  So, a shorter-term fix is to LifoAlloc::mark/release before and after parsing every function (which would limit the total parse node memory consumed to the size of a single function).  I haven't thought through all the ramifications of this (have to audit for any dangling pointers into these released nodes), but I think it'd be relatively straightforward.

	Luke Wagner [:luke] Assignee
	Comment 2 • 11 years ago

Rebased on top of bug 883175 (which ended up simplifying all the maybeReturnType stuff; now a Signature always has a return type).

	Luke Wagner [:luke] Assignee
	Comment 3 • 11 years ago

Woohoo, it works!  Patches soon after I tidy up.

On Epic Citadel, ParseNode memory usage drops from 881MB to 6MB.  It also saves 1.5s total parse+compile time (on my slow MBP).

Note: with these patches, parse time is now included in the time reported by "Successfully compiled asm.js", so the reported times actually go up; don't be fooled.

	Alon Zakai (:azakai)
	Comment 4 • 11 years ago

(In reply to Luke Wagner [:luke] from comment #3)
> Woohoo, it works!  Patches soon after I tidy up.
> 
> On Epic Citadel, ParseNode memory usage drops from 881MB to 6MB.  It also
> saves 1.5s total parse+compile time (on my slow MBP).
> 

\o/

	Luke Wagner [:luke] Assignee
	Comment 5 • 11 years ago

This is a combined patch that applies to trunk (0252b45289f5).  I'd really appreciate fuzzing (iiuc, there are now 3 :).  The patch touches the frontend and thus must contain a bug involving the simultaneous use of arrow functions, with, generator expressions and lazy parsing.

	Luke Wagner [:luke] Assignee
	Comment 6 • 11 years ago

If asm.js validation fails, we have to backup and reparse, just like we already do when we discover "use strict".  This patch wraps 'strict' in a 'Directives' struct that preserves existing behavior so that the last patch can simply add a second 'bool asmJS' field to Directive.

	Luke Wagner [:luke] Assignee
	Comment 7 • 11 years ago

This is just a rebase of the already-posted patch.

	Luke Wagner [:luke] Assignee
	Comment 8 • 11 years ago

No reason that I can see to wait to set it at the end and setting it early simplifies the last patch.

	Luke Wagner [:luke] Assignee
	Comment 9 • 11 years ago

This patch adds ParseContext::maybeFunction which is a pointer to the PNK_FUNCTION ParseNode (if pc->sc->isFunctionBox()).  Otherwise there is no good way to get to your enclosing PNK_FUNCTION when you are parsing a statement in that function (as we are in maybeParseDirective in the last patch).

	Luke Wagner [:luke] Assignee
	Comment 10 • 11 years ago

This patch moves the local bool var 'destructuringArg' into FunctionBox so that we can tell, after Parser::functionArguments(), whether we have destructuring arguments.  Before the last patch, we trigger a validation error because of the extra parse nodes before "use asm" that get inserted; however these nodes are only inserted after parsing the body completes so that are not available to CompileAsmJS and we need the flag.

	Luke Wagner [:luke] Assignee
	Comment 11 • 11 years ago

This patch does the deed; nothing too tricky given all the preceding patches.  I'd appreciate careful consideration of anywhere I explicitly touch the tokenStream.

	Luke Wagner [:luke] Assignee
	Comment 12 • 11 years ago

Oops, missed this one.  If there isn't already an active IonContext, MacroAssembler()'s constructor will create one and use cx->tempLifoAlloc() as the LifoAlloc.  This interacts poorly with parsing which also uses cx->tempLifoAlloc.  This patch adds a new constructor for MacroAssembler that never creates an IonContext.  This is nice since we get a clean JS_ASSERT anywhere where we depend on an IonContext where we haven't explicitly pushed one in AsmJS.cpp (there was one, which this patch fixes).

	Luke Wagner [:luke] Assignee
	Comment 13 • 11 years ago

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=0faeb5cea7ea

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 14 • 11 years ago

(In reply to Luke Wagner [:luke] from comment #5)
> Created attachment 769156 [details] [diff] [review]
> combined patch for fuzzing v.1 (applies to cset 0252b45289f5)

parse("\
    ({\
        r: function() {}\
        (function(){\
        \"use asm\";\
        function f() {}\
        return f\
")

Crash [@ ModuleCompiler::staticallyLink]

parse("\
    function e() {\
        \"use asm\";\
        return\
        )\
")

Crash [@ CheckModule]

tested on 64-bit deterministic threadsafe debug shell, patched on top of m-c rev 942686767e5e.

	Luke Wagner [:luke] Assignee
	Comment 15 • 11 years ago

Oops, bug involving shell-only Parse() method.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 17 • 11 years ago

Function("\
    \"use asm\";\
    var z=-2w;\
")

Crash [@ js::frontend::TokenStream::getTokenInternal]

	Luke Wagner [:luke] Assignee
	Comment 18 • 11 years ago

Ah hah, the VarStmtIter interface obscured parse errors so getToken was called after an error was thrown.  Added more tests for parse errors.

	Luke Wagner [:luke] Assignee
	Comment 19 • 11 years ago

Thanks Gary!

	Christian Holler (:decoder)
	Comment 20 • 11 years ago

Comment on attachment 769319 [details] [diff] [review]
combined patch for fuzzing v.3 (applies to cset 0252b45289f5)

Tests work with 32 bit debug shells, no options required:

g = { z: function() { "use asm"; function g() {} function g () {} } }

Assertion failure: !bound(), at ../ion/shared/Assembler-shared.h:234


(function() {
  'use asm';
  function _main() {
    var label=0;
      switch (label | 0) {
       case 1:
       case (100000000):
  }
})()

Assertion failure: curBlock_ == __null, at js/src/ion/AsmJS.cpp:1749



Overall it's very important to get more ASM.js tests into our testsuite. The existing jit-tests are very valuable, with the big exception of the initial test set that uses eval/strings for everything. These tests are completely useless for LangFuzz. So basically the LangFuzz fuzzing is solely relying on ASM.js regression tests being checked in.

	Luke Wagner [:luke] Assignee
	Comment 21 • 11 years ago

Thanks!  One is an assertion that incorrectly fires on OOM, the other was a preexisting bug covered by the old parsing scheme.

	Luke Wagner [:luke] Assignee
	Comment 22 • 11 years ago

Now explicitly check for duplicates instead of relying on parser.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 23 • 11 years ago

Comment on attachment 769912 [details] [diff] [review]
combined patch for fuzzing v.4 (applied to cset 95e83b0f7cb2)

Didn't find any more problems related to this patch after a round of overnight fuzzing this time.

	Christian Holler (:decoder)
	Comment 24 • 11 years ago

Comment on attachment 769912 [details] [diff] [review]
combined patch for fuzzing v.4 (applied to cset 95e83b0f7cb2)

Same here :)

	Nicholas Nethercote [inactive]
	Comment 25 • 11 years ago

Luke: how Odin-specific is this?

	Luke Wagner [:luke] Assignee
	Comment 26 • 11 years ago

Totally (hence the "OdinMonkey:" prefix).  Good news though: in the non-Odin case, we already (as of bug 678037) avoid the memory spike since all the top-level functions will get a syntax-only parse (which doesn't create parse nodes).

	Luke Wagner [:luke] Assignee
	Comment 27 • 11 years ago

Thanks Gary and Christian!

	Sean Stangl [:sstangl]
	Comment 28 • 11 years ago

Comment on attachment 769192 [details] [diff] [review]
Part 1b: avoid creating an IonContext in MacroAssembler

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

::: js/src/ion/IonMacroAssembler.h
@@ +108,5 @@
>          m_buffer.id = GetIonContext()->getNextAssemblerId();
>  #endif
>      }
>  
> +    // asm.js compilation handles it's own IonContet-pushing

"pushes its own IonContext"

	Douglas Crosher [:dougc]
	Comment 29 • 11 years ago

FWIW an attempt to rebase.  There are a few clashes with bug 885758 that could use some more attention.  An attempt has been made to get the 'perf' support rebased, and I'll keep developing and testing this.

	Douglas Crosher [:dougc]
	Comment 30 • 11 years ago

Some of the issues encountered appear to be general regressions from other work and have been moved to separate bugs: Bug 893314, Bug 893315, and Bug 893317. This rebased patch assumes the patches from these other bugs have been applied. It still seems to work on x64, and passes all the tbpl jit-tests.

	Douglas Crosher [:dougc]
	Comment 31 • 11 years ago

Fix some missing include files that are needed for a non-debug build.

Now also tested on ARM B2G and Firefox-for-Android and all is well so far.

Would be curious to know how to measure the difference this makes to the memory allocation?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 32 • 11 years ago

For either of them, I usually just run ps (or top -m 5 or something) every half second or so via adb shell and watch the numbers.  At one point I had a script that would graph them, but have no idea where it went..

	Jason Orendorff [:jorendorff]
	Comment 33 • 11 years ago

Comment on attachment 769164 [details] [diff] [review]
Part 0: generalize how we reparse for strict mode

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

This is all a bit more of a maze than I'd like, but I don't see a more localized way to do it.

Someday soon I definitely want to collect all the function-parsing code in a contiguous block of craziness at the end of Parser.cpp and go through it with a red pencil (or machete).

::: js/src/frontend/Parser-inl.h
@@ +41,5 @@
>  }
>  
>  template <typename ParseHandler>
>  inline
> +ParseContext<ParseHandler>::ParseContext(Parser<ParseHandler> *prs, GenericParseContext *parent, 

trailing whitespace on this line

::: js/src/frontend/Parser.cpp
@@ +2104,5 @@
>  template <>
>  bool
>  Parser<SyntaxParseHandler>::functionArgsAndBody(Node pn, HandleFunction fun,
>                                                  FunctionType type, FunctionSyntaxKind kind,
> +                                                Directives directives,

Style nit: In the places where this is a parameter, it could be "inheritedDirectives", especially if the idea is that "use asm" is not inherited.

::: js/src/frontend/Parser.h
@@ +205,5 @@
> +    // In a function context, points to a Directive struct that can be updated
> +    // to reflect new directives encountered in the Directive Prologue that
> +    // require reparsing the function. In global/module/generator-tail
> +    // contexts, where there is no DirectivePrologue or need to reparse, this
> +    // pointer may be NULL;

Funny punctuation at the end there.

The comment is a bit misleading. Global contexts can have a DirectivePrologue, we just don't reparse for them.

::: js/src/frontend/SharedContext.h
@@ +152,5 @@
> +    void setStrict() { strict_ = true; }
> +    bool strict() const { return strict_; }
> +
> +    Directives &operator=(Directives rhs) {
> +        // Assignment must be monotonic to prevent reparsing iloops

Mmm, this is a kind of weird place to assert this. It could be checked in the loop itself instead, if you agree. No big deal either way.

	Luke Wagner [:luke] Assignee
	Comment 34 • 11 years ago

Comment on attachment 769165 [details] [diff] [review]
Part 1: change Odin to make a single pass over the parse tree (updated)

Benjamin, I think you have the most familiarity with the AsmJS code atm.  Jason is pretty busy with ES work, so could you please review this?  I'm sorry the patch is kinda big, but logically the change is pretty simple: we stop storing ParseNode* to function's we've already changing how we lookup and record function signatures.

	Luke Wagner [:luke] Assignee
	Comment 35 • 11 years ago

Part 0 (leave open):
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a84be1a35c

	Brian Hackett [Laid off!]
	Comment 36 • 11 years ago

Comment on attachment 769169 [details] [diff] [review]
Part 2: set pn->pn_funbox immediately

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

::: js/src/frontend/Parser.cpp
@@ +534,5 @@
>          return NULL;
>      }
>  
>      traceListHead = funbox;
> +    if (fn != ParseHandler::null())

Will just 'if (fn)' work here?

	Luke Wagner [:luke] Assignee
	Comment 37 • 11 years ago

Oops, wrong patch attached.

	Luke Wagner [:luke] Assignee
	Comment 38 • 11 years ago

Rebased to tip.

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

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

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 40 • 11 years ago

Comment on attachment 776561 [details] [diff] [review]
Part 1: change Odin to make a single pass over the parse tree (rebased)

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

Looks good to me! r+ with the TryEnablingFunction fix.

::: js/src/ion/AsmJS.cpp
@@ +35,5 @@
> +using namespace mozilla;
> +static const size_t LIFO_ALLOC_PRIMARY_CHUNK_SIZE = 1 << 12;
> +
> +using namespace js;
> +using namespace js::frontend;

Several |using namespace| seem to be written twice: js, js::frontend, mozilla.

@@ +1467,5 @@
>  
>          unsigned lineno = 0U, columnIndex = 0U;
>          tokenStream_.srcCoords.lineNumAndColumnIndex(func.fn()->pn_pos.begin, &lineno, &columnIndex);
>  
>          unsigned startCodeOffset = func.codeLabel()->offset();

If I understand correctly the change just above, changes are required here and in trackPerfProfiledBlocks too.

@@ +1628,5 @@
>  {
>    public:
>      struct Local
>      {
>          enum Which { Var, Arg } which;

Looks like this enum is not used any more.

@@ +1644,5 @@
>      typedef Vector<ParseNode*, 4> NodeStack;
>  
>      ModuleCompiler &       m_;
> +    LifoAlloc &            lifo_;
> +    ParseNode              *fn_;

Nit: |ParseNode * [spaces] fn_;| would be more consistent with the declarations above and below.

@@ +3424,5 @@
>      return true;
>  }
>  
>  static bool
> +CheckSignatureAgainstExisting(ModuleCompiler &m, ParseNode *usepn, const Signature &sig,

It's kind of sad that we cannot reuse == here without losing all the error reporting.

@@ +5447,1 @@
>          return true;

Shouldn't we still test |if (!script)| here?
Currently, this |return true| isn't bound to any condition, so Ion won't ever be enabled.

::: js/src/ion/AsmJS.h
@@ +144,5 @@
>      ion::LIRGraph *lir;     // Passed from worker to main thread.
>      unsigned compileTime;
>  
>      AsmJSParallelTask(size_t defaultChunkSize)
> +      : lifo(defaultChunkSize), mir(NULL), lir(NULL), compileTime(0)

Could you initialize func to NULL too, please?

::: js/src/ion/AsmJSModule.h
@@ +496,2 @@
>      bool addExit(unsigned ffiIndex, unsigned *exitIndex) {
> +        if (SIZE_MAX - funcPtrTableAndExitBytes_ < sizeof(void*))

Just to be consistent with the addition below, shouldn't it be sizeof(ExitDatum) here?

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 41 • 11 years ago

Comment on attachment 776560 [details] [diff] [review]
Part 5: LifoAlloc::release after parsing every function

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

Nice, looks good to me!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4523,1 @@
>                 .setForEval(false)

Nit: alignment is broken here.

::: js/src/ion/AsmJS.cpp
@@ +2456,5 @@
>      }
>  
>      /*************************************************************************/
>  
> +    MIRGenerator *extractMIR()

I like this name better too.

::: js/src/ion/AsmJS.h
@@ +41,2 @@
>  
> +// Implements an asm.js module function that has been successfully validated.

The previous comment seemed more explicit, IMHO.

	Luke Wagner [:luke] Assignee
	Comment 42 • 11 years ago

(In reply to Benjamin Bouvier [:bbouvier] from comment #40)
Whoa, nice find in TryEnablingIon!  That is what happens when you hg pull --rebase :)  Also, really nice comments overall, thanks!

	Luke Wagner [:luke] Assignee
	Comment 43 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/63d537758124
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae936372107
https://hg.mozilla.org/integration/mozilla-inbound/rev/d112cd2746c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3f61b13984
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea0b122e8b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/541c383c1698

	Alon Zakai (:azakai)
	Comment 44 • 11 years ago

Nice speedups on workload0 (pure startup time) on large benchmarks on awfy, http://arewefastyet.com/#machine=11&view=breakdown&suite=asmjs-apps

Happens on odin and non-odin (is that surprising? i thought this was odin-specific).

	Luke Wagner [:luke] Assignee
	Comment 45 • 11 years ago

Yes, that is surprising... I'm guessing there was an issue involving lazy parsing whereby we were either parsing more than once or decompressing multiple times.

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/63d537758124
https://hg.mozilla.org/mozilla-central/rev/4ae936372107
https://hg.mozilla.org/mozilla-central/rev/d112cd2746c4
https://hg.mozilla.org/mozilla-central/rev/ae3f61b13984
https://hg.mozilla.org/mozilla-central/rev/eea0b122e8b5
https://hg.mozilla.org/mozilla-central/rev/541c383c1698

	Eddie Maddox
	Comment 47 • 11 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> Target Milestone: --- → mozilla25
With Ff24 being an ESR, this means an extra year to realize the
~100 times reduction in that memory usage spike for managed users.

Is it too risky to uplift this now?
Or is there just no interest/awareness?

Thanks,
Eddie Maddox

	lwagner
	Comment 48 • 11 years ago

I think the patch is way to big and risky to uplift.  Also note that the optimization only applies to asm.js which isn't yet widespread.

	Nicholas Nethercote [inactive]
	Comment 49 • 11 years ago

I've been using a version of one of the Unreal demos to measure asm.js parsing speed.  I have a 55 MiB file called unreal.data, and then I have unreal.js which is just this:

  parse(readRelativeToScript("unreal.data"));

On my 64-bit desktop, with current mozilla-inbound tip, I see a ParseNode peak of 
1,302,290,432 bytes.

parse() does a full parse, not a "syntax" parse, so I figure that must be the difference between what Luke saw in the browser and what I see in the shell?

	Luke Wagner [:luke] Assignee
	Comment 50 • 11 years ago

Not quite: parse() calls Parser::parse() which isn't a "real" parsing function and, among other things, doesn't create a ScriptSource which ends up completing avoiding the asm.js path (see Parser<FullParseHandler::asmJS).  This ends up being kinda nice since JS_BufferIsCompileableUnit uses Parser::parse() so this avoids compiling asm.js (and emitting the "success" message) twice on the REPL.

(To wit, asm.js aborts the syntax-only parse (for reasoning explaining in Parser<SyntaxParseHandler>::asmJS) so that asm.js compilation always occurs during a full parse.)

	Nicholas Nethercote [inactive]
	Comment 51 • 11 years ago

If I want to measure the start-up time (parsing, compilation, whatever else) of an asm.js program from the shell, is there a good way to do it?  Would compile() do it?  Alternatively, Marty suggested for the Unreal demo just removing the run() call at the end and then executing it normally.

Likewise, for normal JS code is there a better way to measure parsing time than with parse()?

	Luke Wagner [:luke] Assignee
	Comment 52 • 11 years ago

For asm.js, yeah, I tend to just download the entire file and just run it; it tends to hit a runtime error (for lack of document/window) really quickly.  For non-asm.js, parse() is great.