Closed Bug 1358501 Opened 4 years ago Closed 2 years ago

Investigate spreadcall optimization in Ion

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: tcampbell, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: es6perf
Priority: -- → P3
Attached patch bug1358501.patch (obsolete) — Splinter Review

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.

Assignee: nobody → andrebargull
Attachment #9043877 - Flags: feedback?(jdemooij)
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.
Attachment #9043877 - Flags: feedback?(jdemooij) → feedback+

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

Attached patch bug1358501.patchSplinter Review

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.

Attachment #9043877 - Attachment is obsolete: true
Attachment #9044650 - Flags: review?(jdemooij)
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!
Attachment #9044650 - Flags: review?(jdemooij) → review+

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

Pushed by apavel@mozilla.com:
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

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.