1028262 - SpiderMonkey arguments analysis doesn't consider MGetArgumentsObjectArg

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Kannan Vijayan [:djvj]]

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.

Comment 1

[Caio Lima(:caiolima)]

Attachment: 1028262.patch

Considering MGetArgumentsObjectArg that can be lazy

Comment 2

[Kannan Vijayan [:djvj]]

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).

Comment 3

[Caio Lima(:caiolima)]

Attachment: 1028262.patch

Fixed the comment

Comment 4

[Kannan Vijayan [:djvj]]

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.

Comment 5

[Caio Lima(:caiolima)]

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.

Comment 6

[Till Schneidereit [:till]]

Attachment: 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.)

Comment 7

[Till Schneidereit [:till]]

Attachment: patch, v4 with fixed description

Comment 8

[Till Schneidereit [:till]]

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"

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/649013d75bca

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/649013d75bca