create arguments object eagerly

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla14
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: ashuk → general
Component: Java APIs for DOM → JavaScript Engine
QA Contact: dom-apis → general
(Assignee)

Comment 1

5 years ago
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #603914 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Depends on: 730497
(Assignee)

Comment 2

5 years ago
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?
Attachment #603915 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
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.
Attachment #603916 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 718134
(Assignee)

Comment 5

5 years ago
Created attachment 603946 [details] [diff] [review]
reoptimize f.apply(arguments)

Oops, wrong version
Attachment #603916 - Attachment is obsolete: true
Attachment #603946 - Flags: review?(bhackett1024)
Attachment #603916 - Flags: review?(bhackett1024)
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.
Attachment #603914 - Flags: review?(bhackett1024) → review+
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.
Attachment #603915 - Flags: review?(bhackett1024)
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 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.
Attachment #603946 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 10

5 years ago
(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.
Blocks: 735406
(Assignee)

Comment 11

5 years ago
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.
Attachment #605615 - Flags: review?(bhackett1024)
Attachment #605615 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 12

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/85bef04d1258
https://hg.mozilla.org/mozilla-central/rev/e2144e6ee774
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
Depends on: 737575
(Assignee)

Updated

5 years ago
Depends on: 737388
(Assignee)

Updated

5 years ago
Depends on: 738083
Depends on: 769987
You need to log in before you can comment on or make changes to this bug.