Implement JSOP_CALLSITEOBJ support in the JITs

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gupta.rajagopal, Assigned: gupta.rajagopal)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Created attachment 8457421 [details] [diff] [review]
Implement JSOP_CALLSITEOBJ support in the JITs v1
Attachment #8457421 - Flags: review?(jorendorff)
Comment on attachment 8457421 [details] [diff] [review]
Implement JSOP_CALLSITEOBJ support in the JITs v1

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

Bouncing the review to Jan.

It looks good to me.

If you haven't already, run the test with the IONFLAGS environment variable set, like:
    IONFLAGS=aborts,scripts,bl-scripts,bl-scripts $JS jit-test/tests/basic/tagTempl.js
and make sure things are more or less what you expect. Ask in #jsapi for help if the IONFLAGS output doesn't make sense.

You can use
    IONFLAGS=help $JS
to get a short explanation of each flag, but those 4 are the most useful for what you're doing, I think.

::: js/src/jit-test/tests/basic/tagTempl.js
@@ +10,5 @@
> +    function func(a) { return a; }
> +
> +    for (var i = 0; i < 10000; i++) {
> +        if (i === 0 || i === 25 || i === 9999)
> +            cso[index++] = func`hey${4}there`;

This will be a syntax error if hasTemplateStrings is false.

My vote is: land this after enabling template strings permanently, so we don't need these hacks anymore.

Not crazy about the magic numbers; why not just call it every time through? The JIT should inline the function; it should be really fast.

::: js/src/jit/BaselineCompiler.cpp
@@ +1267,5 @@
> +BaselineCompiler::emit_JSOP_CALLSITEOBJ()
> +{
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +    frame.push(ObjectValue(*script->getObject(pc)));
> +#endif

JSOP_CALLSITEOBJ is defined even if JS_HAS_TEMPLATE_STRINGS is false, so drop the #ifdef here.
Attachment #8457421 - Flags: review?(jorendorff) → review?(jdemooij)
Comment on attachment 8457421 [details] [diff] [review]
Implement JSOP_CALLSITEOBJ support in the JITs v1

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

Looks good, r=me with comments addressed.

::: js/src/jit-test/tests/basic/tagTempl.js
@@ +9,5 @@
> +
> +    function func(a) { return a; }
> +
> +    for (var i = 0; i < 10000; i++) {
> +        if (i === 0 || i === 25 || i === 9999)

Agreed with Jason on removing the magic numbers and I'd also lower the iteration count from 10000 to 2000 (even 2000 is pretty high, we also run jit-tests with --ion-eager and --baseline-eager).

I'd also move this loop into its own function and call that instead. The global script has an eval() and a try-catch, and although Ion should compile those in this case it's best to avoid having them in the same script (and function code is also faster than global code, especially when there's no eval, so your loop will run even faster).

::: js/src/jit/IonBuilder.cpp
@@ +1711,5 @@
>  
>        case JSOP_REGEXP:
>          return jsop_regexp(info().getRegExp(pc));
>  
> +#ifdef JS_HAS_TEMPLATE_STRINGS

If there's no #ifdef in the interpreter/Baseline, let's drop the one here too.

@@ +1714,5 @@
>  
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +      case JSOP_CALLSITEOBJ:
> +        pushConstant(ObjectValue(*(info().getObject(pc))));
> +        return true;

Nit: pushConstant returns a bool so you can just:

return pushConstant(...);
Attachment #8457421 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 8458840 [details] [diff] [review]
Implement JSOP_CALLSITEOBJ support in the JITs v2
Attachment #8457421 - Attachment is obsolete: true
Attachment #8458840 - Flags: review+
(Assignee)

Comment 5

4 years ago
Created attachment 8463725 [details] [diff] [review]
Implement JSOP_CALLSITEOBJ support in the JITs v3

Moved BaselineCompiler changes from Bug 1031397 to here.
Attachment #8458840 - Attachment is obsolete: true
Attachment #8463725 - Flags: review+
(Assignee)

Comment 6

4 years ago
Created attachment 8464826 [details] [diff] [review]
Implement JSOP_CALLSITEOBJ support in the JITs v4

Try run fixes.

https://tbpl.mozilla.org/?tree=Try&rev=e853ffe398d4
Attachment #8463725 - Attachment is obsolete: true
Attachment #8464826 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
That try run has a single canceled build. Clearing checkin-needed until we a finished try run is posted.
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
(Assignee)

Comment 8

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ec5534acd24a
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7019f410c55b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.