876434 - BaselineCompiler: Compile JSOP_CALLEE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Ion already compiles it (not currently though because baseline aborts) and JSOP_CALLEE isn't that uncommon.

Comment 1

[Tom Schuster [:evilpie]]

Seems like we would never ion compile JSOP_CALLEE in 23. I think we had some performance fault with that in the past, maybe we should consider uplifting to aurora.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 754474 [details] [diff] [review]
Patch

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

Good catch.  This would have prevented ion compilation of CALLEE functions when baseline is enabled.

I agree with Tom, we should see if we can uplift into 23.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Kannan Vijayan [:djvj] from comment #2)
 
> I agree with Tom, we should see if we can uplift into 23.

It would help us to evaluate an uplift request if you filled out the form that comes up when you set the ? - mainly what is the risk involved here and what is the user impact if this is not uplifted?

Comment 4

[Jan de Mooij [:jandem]]

I think we should consider uplifting after it has been in a nightly or two.

https://hg.mozilla.org/integration/mozilla-inbound/rev/61ec49c34885

Comment 5

[Kannan Vijayan [:djvj]]

(In reply to [e-mail] [:lsblakk] from comment #3)
> (In reply to Kannan Vijayan [:djvj] from comment #2)
>  
> > I agree with Tom, we should see if we can uplift into 23.
> 
> It would help us to evaluate an uplift request if you filled out the form
> that comes up when you set the ? - mainly what is the risk involved here and
> what is the user impact if this is not uplifted?

Sorry, will do once it's gone through a couple of nightlies as Jan suggests.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61ec49c34885

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Kannan Vijayan [:djvj] from comment #5)
> 
> Sorry, will do once it's gone through a couple of nightlies as Jan suggests.

Now that it has been in some nightlies - any new info to share?

Comment 8

[Kannan Vijayan [:djvj]]

Comment on attachment 754474 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
N/A

User impact if declined:
Low performance.  Any function which uses JSOP_CALLEE (e.g. recursive functions) will not be ion-compiled.

Testing completed (on m-c, etc.): 
Made it through a couple of nightlies without any major issues reported.

Risk to taking this patch (and alternatives if risky):
Very low.  Baseline codegeneration for this is trivial, and the Ion version already exist and has been tested.

String or IDL/UUID changes made by this patch:
None.

Comment 9

[Kannan Vijayan [:djvj]]

Thanks for the ping Lukas.  Aurora approval form filled out.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b5e5b168e78