IsRegExpObject and RegExpInstanceOptimizable not always inlined in Speedometer

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

4 months ago
IsRegExpObject
- Not inlined in Backbone because this check fails <http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/js/src/jit/MCallOptimize.cpp#2172>.
- Not inlined in AngularJS because it's called with MIRType::Value <http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/js/src/jit/MCallOptimize.cpp#2167>. 

RegExpInstanceOptimizable
- Not inlined in React-Redux because it's called with MIRType::Value <http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/js/src/jit/MCallOptimize.cpp#2220>.
(Assignee)

Comment 1

4 months ago
Created attachment 8903589 [details] [diff] [review]
bug1395927-part1-intrinsic-argc-checks.patch

A couple of intrinsic functions already used assertions, this adds assertions for all intrinsic functions:

Intrinsic functions mustn't be called with the wrong number of arguments, so let's just assert the arguments count is correct, which saves us unnecessary checks in release builds.
Attachment #8903589 - Flags: review?(jdemooij)
(Assignee)

Comment 2

4 months ago
Created attachment 8903590 [details] [diff] [review]
bug1395927-part2-type-checks.patch

The object-type checks were necessary before the ES6 RegExp changes, nowadays we only need to check the argument is a, possibly boxed, string.
Attachment #8903590 - Flags: review?(jdemooij)
(Assignee)

Comment 3

4 months ago
Created attachment 8903591 [details] [diff] [review]
bug1395927-part3-hasclass-for-isregexp.patch

This change allows us to inline IsRegExpObject more often in React-Redux (reduces the non-inlined calls to intrinsic_IsInstanceOfBuiltin<js::RegExpObject> from ~16000 to ~2000-4000 in a single Speedometer/React-Redux run). 

The proxy-check isn't necessary for RegExp-objects, probably it was just copied over from IonBuilder::inlineIsCallable(), so we can actually use MHasClass to test for RegExp-objects even when |clasp| is nullptr.
Attachment #8903591 - Flags: review?(jdemooij)
(Assignee)

Comment 4

4 months ago
Created attachment 8903594 [details] [diff] [review]
bug1395927-part4-isregexp-with-value.patch

This change allows us to inline IsRegExpObject more often in AngularJS (reduces the non-inlined calls to intrinsic_IsInstanceOfBuiltin<js::RegExpObject> from ~17000-20000 to ~2000-3000 in a single Speedometer/AngularJS run). 

I've reordered the statement to match the statement order in IonBuilder::inlineIsCallable(), because both methods perform similar checks, so I think it makes sense to perform these checks in the same sequence.

MHasClass uses SingleObjectPolicy for its argument, but only allowed arguments with type MIRType::Object. For the use case in inlineIsRegExpObject, I had to relax the assertion in MHasClass' constructor to also allow MIRType::Value, I hope that's okay to do.
Attachment #8903594 - Flags: review?(jdemooij)
(Assignee)

Comment 5

4 months ago
Created attachment 8903598 [details] [diff] [review]
bug1395927-part5-regexp-instance-opt-with-value.patch

Another change for AngularJS, it reduces the non-inlined calls to js::RegExpInstanceOptimizable from ~20000 to ~2000 in a single Speedometer/AngularJS run.

The callers to RegExpInstanceOptimizable in self-hosted JS always pass an object to RegExpInstanceOptimizable, but apparently it's sometimes a boxed value, so we need to relax the inlining check to also allow MIRType::Value for |rxArg|.
Attachment #8903598 - Flags: review?(jdemooij)
Comment on attachment 8903589 [details] [diff] [review]
bug1395927-part1-intrinsic-argc-checks.patch

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

Makes sense. If any of these things change we should consider changing this code instead of silently breaking inlining.
Attachment #8903589 - Flags: review?(jdemooij) → review+

Updated

3 months ago
Attachment #8903590 - Flags: review?(jdemooij) → review+
Comment on attachment 8903591 [details] [diff] [review]
bug1395927-part3-hasclass-for-isregexp.patch

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

Nice!
Attachment #8903591 - Flags: review?(jdemooij) → review+

Updated

3 months ago
Attachment #8903594 - Flags: review?(jdemooij) → review+
Comment on attachment 8903598 [details] [diff] [review]
bug1395927-part5-regexp-instance-opt-with-value.patch

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

Great improvements, thanks for doing this.
Attachment #8903598 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 9

3 months ago
I forgot to ask about this for part 4:
I was wondering if we need to mark MHasClass as non-movable when called with MIRType::Value to avoid a deoptimization in case Ion decides to move MHasClass before the MIsObject check (*)? Or is Ion never allowed to swap the execution order of two movable operations?

(*) IsRegExpObject() is always preceded by an IsObject() check in self-hosted code.
(Assignee)

Comment 10

3 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f757ace01ed8acdd25a22968507abe4c74c8525
Keywords: checkin-needed

Comment 11

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e429fee90bf
Part 1: Replace argument count tests with assertions in MCallOptimize for intrinsic functions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6665f8db3a80
Part 2: Replace old type checks to use current expected types. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d84c57174485
Part 3: Inline IsRegExpObject with HasClass. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a57ed979fa
Part 4: Inline IsRegExpObject when called with MIRType::Value. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0998107850f7
Part 5: Inline RegExpInstanceOptimizable when called with MIRType::Value. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e429fee90bf
https://hg.mozilla.org/mozilla-central/rev/6665f8db3a80
https://hg.mozilla.org/mozilla-central/rev/d84c57174485
https://hg.mozilla.org/mozilla-central/rev/a3a57ed979fa
https://hg.mozilla.org/mozilla-central/rev/0998107850f7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.