Closed Bug 1055337 Opened 6 years ago Closed 6 years ago

don't do name resolution while parsing asm.js

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(4 files)

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).
Attached patch rm-dead-argSplinter Review
First, let's remove a dead arg.
Attachment #8474881 - Flags: review?(jorendorff)
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.)
Attachment #8474886 - Flags: review?(jorendorff)
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).
Attachment #8474956 - Flags: review?(jorendorff)
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.
Attachment #8474957 - Flags: review?(jorendorff)
Comment on attachment 8474881 [details] [diff] [review]
rm-dead-arg

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

Sorry for the slow reviews here.
Attachment #8474881 - Flags: review?(jorendorff) → review+
Attachment #8474956 - Flags: review?(jorendorff) → review+
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.)
Attachment #8474886 - Flags: review?(jorendorff) → review+
Attachment #8474957 - Flags: review?(jorendorff) → review+
Thanks Jason!
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).
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".
You need to log in before you can comment on or make changes to this bug.