Closed Bug 1028262 Opened 10 years ago Closed 10 years ago

SpiderMonkey arguments analysis doesn't consider MGetArgumentsObjectArg

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: djvj, Assigned: caiolima)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 1024609
Attached patch 1028262.patch (obsolete) — Splinter Review
Considering MGetArgumentsObjectArg that can be lazy
Attachment #8443611 - Flags: review?(kvijayan)
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+
Attached patch 1028262.patch (obsolete) — Splinter Review
Fixed the comment
Attachment #8443611 - Attachment is obsolete: true
Attachment #8443677 - Flags: review?(kvijayan)
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+
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.
Attached patch patch, v3 (obsolete) — Splinter Review
(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
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: