1055337 - don't do name resolution while parsing asm.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Normally, we do name resolution while parsing.  asm.js validation maintains its own symbol tables, so all the extra symbol-table management done while parsing is redundant and unused (asm.js validation is stricter than parsing; on validation failure, the module is reparsed from the beginning).  These patches make turn off name resolution while parsing asm.js.  On Unity, this saves about 300ms (14% of parse time) and on UE4 this saves about 200ms (10% of parse time).

Comment 1

[Luke Wagner [:luke]]

Attachment: rm-dead-arg

First, let's remove a dead arg.

Comment 2

[Luke Wagner [:luke]]

Attachment: simplify-asm.js-parser

The effect of this patch is to make useAsmOrInsideUseAsm true when parsing asm.js, but false if asm.js validation fails and we are re-parsing.  This makes useAsmOrInsideUseAsm the right predicate to use for the optimizations in the next patch.

(As a bit of background: the way we trigger re-parsing in the parser is to modify newDirectives so that newDirectives!=directives in functionBody.  Thus, as the comment in Parser::asmJS explains, we set newDirectives.asmJS=true if asm.js validation fails.)

Comment 3

[Luke Wagner [:luke]]

Attachment: more-disable-syntax-parser

In writing a test-case for legacy generator expressions (to get at the PN_NAME case of the transplanter), I learned, even from within a full parse, we can still need to restart if we get to certain weird constructs and we have a non-null syntaxParser.  This causes asm.js validation to fail to emit a warning (because ParseFunction assumes that, if parsing returns 'false', we must have hit a real syntax error that will be reported so it doesn't attempt to restart so there is no need to warn).

Comment 4

[Luke Wagner [:luke]]

Attachment: no-name-resolution

I backed off a more aggressive patch that also avoided symbol table operations when binding vars and args since I was worried this could break some assumptions elsewhere that would be exposed when parsing encountered non-asm.js constructs (since the entire function will finish parsing before returning to Odin).  Uses are 12x more common than args+vars combined, so this trivial patch gives almost all the win.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8474881 [details] [diff] [review]
rm-dead-arg

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

Sorry for the slow reviews here.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8474886 [details] [diff] [review]
simplify-asm.js-parser

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

::: js/src/frontend/Parser.cpp
@@ +503,5 @@
>      generatorKindBits_(GeneratorKindAsBits(generatorKind)),
>      inWith(false),                  // initialized below
>      inGenexpLambda(false),
>      hasDestructuringArgs(false),
> +    useAsm(false),

Please update the comment on this field in SharedContext.h.

(Switching to a multiline comment there would be fine with me.)

Comment 7

[Luke Wagner [:luke]]

Thanks Jason!

Comment 8

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aac5dec0cc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee8ccbd653d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3688aba2b07
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdcdf5ad226a
and then I remembered I hadn't addressed comment 6, so:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da67eb8cb631

Comment 9

[Alon Zakai (:azakai)]

It looks like this may have regressed some of the asm benchmarks on non-odin, by 6% or so (e.g. x86-skinning, x86_64-primes, x86_64-bullet-loadtime, x86-lua-loadtime).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2aac5dec0cc2
https://hg.mozilla.org/mozilla-central/rev/7ee8ccbd653d
https://hg.mozilla.org/mozilla-central/rev/a3688aba2b07
https://hg.mozilla.org/mozilla-central/rev/fdcdf5ad226a
https://hg.mozilla.org/mozilla-central/rev/da67eb8cb631

Comment 11

[Luke Wagner [:luke]]

I can't reproduce the skinning regression (actually, I see a mild speedup).  I can reproduce the load-time issues, though: these are due to the fact that one of these patches disables lazy parsing as soon as "use asm" is encountered and this deoptimizes the non-asm.js case.  If there was no "use asm", there would be no deoptimization, so I don't think this is a real regression; ideally the test harness would test --no-asmjs by removing "use asm".