Closed Bug 1395927 Opened 7 years ago Closed 7 years ago

IsRegExpObject and RegExpInstanceOptimizable not always inlined in Speedometer

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

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>.
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)
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)
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)
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)
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+
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+
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+
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.
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
You need to log in before you can comment on or make changes to this bug.