733950 - create arguments object eagerly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

These patches create arguments objects eagerly or not at all.  They are a prerequisite to bug 659577.  I'd like to land it now that I think we can assume bug 718022 stuck.

Comment 1

[Luke Wagner [:luke]]

Attachment: eager arguments

This patch creates arguments eagerly.  It disables the important fun.apply optimization; a later patch adds it back.

Comment 2

[Luke Wagner [:luke]]

Attachment: rm a check

I was weirded out by what it would mean if we had an ARGUMENTS opcode but no Bytecode so I just asserted it doesn't happen... is this valid?

Comment 3

[Luke Wagner [:luke]]

Attachment: reoptimize f.apply(arguments) optimization

With this patch, we go back to our previous optimization level: zero arguments objects are created in v8.

Comment 5

[Luke Wagner [:luke]]

Attachment: reoptimize f.apply(arguments)

Oops, wrong version

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 603914 [details] [diff] [review]
eager arguments

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

Nice!  Sorry for the late review.

::: js/src/jsscript.h
@@ +422,5 @@
>  #endif
>      bool            callDestroyHook:1;/* need to call destroy hook */
>  
> +    /*
> +     * TODO: describe

Needs a comment.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 603915 [details] [diff] [review]
rm a check

This check for 'code' needs to be left in.  In general, the Bytecode for an opcode will not be present if the opcode is unreachable in the script's CFG, e.g. 'function foo() { return 0; return arguments; }'.  The SSA code will get crash if it is fed SSA values referring to unreachable opcodes.  It would be good to add a comment describing why the check is here.

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 603914 [details] [diff] [review]
eager arguments

Also, this patch looks like it's supposed to have ArgumentsObject.cpp.  Can you attach a separate patch with that file for review?

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 603946 [details] [diff] [review]
reoptimize f.apply(arguments)

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

::: js/src/jsscript.cpp
@@ +1988,5 @@
> +
> +    if (hasAnalysis() && analysis()->ranInference()) {
> +        types::AutoEnterTypeInference enter(cx);
> +        for (unsigned off = 0; off < length; off += GetBytecodeLength(code + off)) {
> +            if (code[off] == JSOP_ARGUMENTS) {

This should also check that analysis->maybeCode(off) != NULL.

Comment 10

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #8)
Thanks for the reviews.  Diff is making it look weird, but what I did (to preserve hg history and minimize rebase pain) was to "hg cp jsfun.cpp vm/ArgumentsObject.cpp" and then delete all the not-arguments-object stuff.  Diff shows it as two separate edits to jsfun.cpp.

Comment 11

[Luke Wagner [:luke]]

Attachment: fix to last patch

While I was waiting for the dependencies to get landed I noticed I'm not actually ensuring the !heavyweight assertion in JSScript::applySpeculationFailed.  I'll fold the fix in to the prev patch, but here's the diff.

Comment 12

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2107141265f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3b804118ef

and then I noticed I cited the wrong bug so I relanded with the correct bug #

https://hg.mozilla.org/integration/mozilla-inbound/rev/85bef04d1258
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2144e6ee774

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/85bef04d1258
https://hg.mozilla.org/mozilla-central/rev/e2144e6ee774