Investigate spreadcall optimization in Ion

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: tcampbell, Assigned: anba)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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
Reporter

Updated

2 years ago
Priority: -- → P3
Assignee

Comment 1

4 months ago
Posted 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...)

Assignee

Comment 4

4 months ago

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+
Assignee

Comment 6

4 months ago

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 8

4 months ago

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

Comment 9

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1532262
You need to log in before you can comment on or make changes to this bug.