Last Comment Bug 733950 - create arguments object eagerly
: create arguments object eagerly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
:
Mentors:
: 718134 (view as bug list)
Depends on: 718022 730497 737388 737575 738083 769987
Blocks: 735406
  Show dependency treegraph
 
Reported: 2012-03-07 16:31 PST by Luke Wagner [:luke]
Modified: 2012-07-11 00:43 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
eager arguments (132.58 KB, patch)
2012-03-07 16:40 PST, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
rm a check (1.11 KB, patch)
2012-03-07 16:42 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
reoptimize f.apply(arguments) optimization (19.87 KB, patch)
2012-03-07 16:42 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
reoptimize f.apply(arguments) (19.92 KB, patch)
2012-03-07 18:35 PST, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
fix to last patch (1.75 KB, patch)
2012-03-13 17:47 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-03-07 16:31:15 PST
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] 2012-03-07 16:40:33 PST
Created attachment 603914 [details] [diff] [review]
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] 2012-03-07 16:42:05 PST
Created attachment 603915 [details] [diff] [review]
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] 2012-03-07 16:42:52 PST
Created attachment 603916 [details] [diff] [review]
reoptimize f.apply(arguments) optimization

With this patch, we go back to our previous optimization level: zero arguments objects are created in v8.
Comment 4 Luke Wagner [:luke] 2012-03-07 17:06:21 PST
*** Bug 718134 has been marked as a duplicate of this bug. ***
Comment 5 Luke Wagner [:luke] 2012-03-07 18:35:49 PST
Created attachment 603946 [details] [diff] [review]
reoptimize f.apply(arguments)

Oops, wrong version
Comment 6 Brian Hackett (:bhackett) 2012-03-10 07:07:40 PST
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 (:bhackett) 2012-03-10 07:12:19 PST
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 (:bhackett) 2012-03-10 07:14:59 PST
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 (:bhackett) 2012-03-10 07:31:00 PST
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] 2012-03-10 12:42:48 PST
(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] 2012-03-13 17:47:52 PDT
Created attachment 605615 [details] [diff] [review]
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.

Note You need to log in before you can comment on or make changes to this bug.