1038498 - Implement JSOP_CALLSITEOBJ support in the JITs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[guptha]

Attachment: Implement JSOP_CALLSITEOBJ support in the JITs v1

Comment 2

[Jason Orendorff [: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.

Comment 3

[Jan de Mooij [:jandem]]

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

Comment 4

[guptha]

Attachment: Implement JSOP_CALLSITEOBJ support in the JITs v2

Comment 5

[guptha]

Attachment: Implement JSOP_CALLSITEOBJ support in the JITs v3

Moved BaselineCompiler changes from Bug 1031397 to here.

Comment 6

[guptha]

Attachment: Implement JSOP_CALLSITEOBJ support in the JITs v4

Try run fixes.

https://tbpl.mozilla.org/?tree=Try&rev=e853ffe398d4

Comment 7

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

That try run has a single canceled build. Clearing checkin-needed until we a finished try run is posted.

Comment 8

[guptha]

https://tbpl.mozilla.org/?tree=Try&rev=ec5534acd24a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7019f410c55b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7019f410c55b