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)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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 6•7 years ago
|
||
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•7 years ago
|
Attachment #8903590 -
Flags: review?(jdemooij) → review+
Comment 7•7 years ago
|
||
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•7 years ago
|
Attachment #8903594 -
Flags: review?(jdemooij) → review+
Comment 8•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f757ace01ed8acdd25a22968507abe4c74c8525
Keywords: checkin-needed
Comment 11•7 years 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
Comment 12•7 years ago
|
||
bugherder |
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•