Closed Bug 1383645 Opened 7 years ago Closed 7 years ago

UnsafeGetReservedSlot not inlined for regexp methods when inlined from string methods

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Test case:
---
(function() {
    var re = ["a", /a/];
    var s = "b";
    for (var i = 0; i < 100000; ++i) {
        s.match(re[i & 1]);
    }
})();
---

Breaking here [1] with
    br MCallOptimize.cpp:2776 if callInfo.args_.mBegin[0]->resultType_!=js::jit::MIRType::Object

shows per |p callInfo.getArg(0)->resultType_| that the first argument is typed with js::jit::MIRType::Value which prohibits inlining. I wonder if we can loose the restriction and allow the case when the first argument is MIRType::Value given that MLoadFixedSlot has SingleObjectPolicy, which seems to unbox objects?

UnsafeGetInt32FromReservedSlot is the sixth most called intrinsic in Speedometer per bug 1365361, and this issue seems to explain at least some of the inlining failures in Speedometer.


[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/jit/MCallOptimize.cpp#2776
Attached patch bug1383645.patch (obsolete) — Splinter Review
I assume MLoadFixedSlot's SingleObjectPolicy takes care of unboxing the value and it's better to have an unbox operation than a call. Is that correct?
Attachment #8890898 - Flags: feedback?(jdemooij)
This reminds me of bug 1366263, CC'ing arai.
Comment on attachment 8890898 [details] [diff] [review]
bug1383645.patch

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

Good find, this makes sense. Should we do the same for SetReservedSlot?
Attachment #8890898 - Flags: review+
Attachment #8890898 - Flags: feedback?(jdemooij)
Attachment #8890898 - Flags: feedback+
Attached patch bug1383645.patchSplinter Review
Updated patch to also handle inlineUnsafeSetReservedSlot, carrying r+ because it's a straight forward change.
Assignee: nobody → andrebargull
Attachment #8890898 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8891634 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #3)
> Comment on attachment 8890898 [details] [diff] [review]
> bug1383645.patch
> 
> Review of attachment 8890898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good find, this makes sense. Should we do the same for SetReservedSlot?

I haven't tested if we fail to inline SetReservedSlot, because the object argument isn't typed as MIRType::Object, but even if it's currently not an issue, I don't see a reason to wait until it becomes a problem. IOW I've updated inlineUnsafeSetReservedSlot to also allow MIRType::Value. :-)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ec4e403b9d
Allow to inline UnsafeGetReservedSlot when the object is typed as MIRType::Value. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1ec4e403b9d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.