1358501 - Investigate spreadcall optimization in Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Ted Campbell [:tcampbell]]

Follow-up to bug 1353358.

When we emit bytecode for spreadcalls of a single argument expansion, we use JSOP_OPTIMIZE_SPREADCALL to detect if we can use the argument directly (instead of following iterator protocol). In interpreter and baseline we use a VMCall to inspect the object as well as special iterator prototypes being unmodified.

Currently in Ion, we avoid this mechanism and assume JSOP_OPTIMIZE_SPREADCALL is false instead of performing an expensive VMCall. We should investigate inlining the appropriate logic to allow Ion to do the check.

Comment 1

[André Bargull [:anba]]

Attachment: bug1358501.patch

f? only because I'm not sure TI can be used this way.

So, can we use TI to create type constraints for objects which aren't part of the current MDefinition in IonBuilder? This passes on Try, but I simply don't know if there are some implicit assumptions built into TI which makes this usage not valid.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 9043877 [details] [diff] [review]
bug1358501.patch

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

Nice idea! Yes this is fine; I didn't look at it in detail but the approach makes sense.

For the final patch it would be good to add some tests that fail if you comment out some of these checks.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)

For the final patch it would be good to add some tests that fail if you
comment out some of these checks.

(Or check we have existing ones that catch this...)

Comment 4

[André Bargull [:anba]]

Attachment: bug1358501.patch

Thanks for the feedback!

I've updated the patch to add tests for the various conditions which need to be checked. Each test has a "static" and "dynamic" version, where "static" tests the case when the JSOP_OPTIMIZE_SPREADCALL condition is already invalid when initially Ion compiling the script and "dynamic" attempts to cover the case when the invalidation happens dynamically.

The predicate function argument for IonBuilder::propertyIsConstantFunction shouldn't be necessary, because when the property is still a constant, the property value should always be the initial, expected JSFunction. I only added the predicate for extra safety and to keep propertyIsConstantFunction a bit more versatile. But I can also change the final check in the return statement (value.isObject() && value.toObject().is<JSFunction>() && test(this, &value.toObject().as<JSFunction>())) into an assertion, if you prefer that.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 9044650 [details] [diff] [review]
bug1358501.patch

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

Sorry for the terrible delay. This looks great, thanks a lot for adding so many test cases!

Comment 6

[André Bargull [:anba]]

No worries. I normally don't mind if reviews take a bit longer, especially if the changes are self-contained and can easily be applied even after some time. :-)

Comment 7

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=013578f09c2d080fed39e05a72505df07bfc5647

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d1480593cf
Ion optimize JSOP_OPTIMIZE_SPREADCALL with a constant when Array iterator properties are in their initial state. r=jandem

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e3d1480593cf