SpiderMonkey arguments analysis doesn't consider MGetArgumentsObjectArg

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: djvj, Assigned: caiolima)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Arguments analysis in Spidermonkey uses IonMonkey to compile a script and then analyze the uses of the arguments object definition.  If there is a non-compatible use, the script is marked as "needsArgsObj"

When considering the uses, the MGetArgumentsArg instruction is not considered to be a valid use.  This leads to argsobj creation in function which don't require it.
(Reporter)

Updated

4 years ago
Blocks: 1024609
(Assignee)

Comment 1

4 years ago
Created attachment 8443611 [details] [diff] [review]
1028262.patch

Considering MGetArgumentsObjectArg that can be lazy
Attachment #8443611 - Flags: review?(kvijayan)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8443611 [details] [diff] [review]
1028262.patch

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

::: js/src/jit/IonAnalysis.cpp
@@ +2355,5 @@
>          *argumentsContentsObserved = true;
>          return true;
>      }
>  
> +    // MGetArgumentsObjectArg needs to be considered as a valid argument(Bug: 1028262)

Drop the bug reference.  It clutters up the code and the same info can be obtained directly from the revision tree (hg blame).
Attachment #8443611 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8443677 [details] [diff] [review]
1028262.patch

Fixed the comment
Attachment #8443611 - Attachment is obsolete: true
Attachment #8443677 - Flags: review?(kvijayan)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8443677 [details] [diff] [review]
1028262.patch

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

Looks good.  For future reference, an r+ with comments generally means you don't need to re-request a review for landing after addressing the comments.
Attachment #8443677 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 5

4 years ago
Kannan, there is any other thing that I need to do to land this patch on the tree? I'm waiting it to resume on bug #1024609.
Created attachment 8445031 [details] [diff] [review]
patch, v3

(In reply to Caio Lima(:caiolima) from comment #5)
> Kannan, there is any other thing that I need to do to land this patch on the
> tree? I'm waiting it to resume on bug #1024609.

I pushed it to the try server: https://tbpl.mozilla.org/?tree=Try&rev=df2d467ad02f

Once that turns up green (which I expect it to), you can set the `checkin-needed` keyword on the bug, and one of the sheriffs will land the patch to mozilla-inbound. You might consider applying for level 1 commit access (http://www.mozilla.org/hacking/commit-access-policy/) which would enable you to push to try yourself.

(note: I very slightly changed the patch to fix a grammar error and remove braces that are not needed for single-line if contents.)
Attachment #8443677 - Attachment is obsolete: true
Attachment #8445031 - Flags: review+
Assignee: nobody → ticaiolima
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 8445179 [details] [diff] [review]
patch, v4 with fixed description
Attachment #8445031 - Attachment is obsolete: true
Attachment #8445179 - Flags: review+
Ok, the try run is green. I uploaded yet another version of the patch because I didn't see the problematic description line last time around. It should have these characteristics:

- be of the format "Bug [bug number] - [description]. r=[reviewer]"
- the description should be about what the patch does, not what the problem is. I.e. in this case it should describe the change done, not the functional area it is done in
- the reviewer has to be mentioned

Taken together, I changed the description line to
"Bug 1028262 - Enable lazy argsobj creation for functions using MGetArgumentsObjectArg. r=djvj"
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/649013d75bca
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.