1383645 - UnsafeGetReservedSlot not inlined for regexp methods when inlined from string methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[André Bargull [:anba]]

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

Comment 1

[André Bargull [:anba]]

Attachment: bug1383645.patch

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?

Comment 2

[Jan de Mooij [:jandem]]

This reminds me of bug 1366263, CC'ing arai.

Comment 3

[Jan de Mooij [:jandem]]

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?

Comment 4

[André Bargull [:anba]]

Attachment: bug1383645.patch

Updated patch to also handle inlineUnsafeSetReservedSlot, carrying r+ because it's a straight forward change.

Comment 5

[André Bargull [:anba]]

(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. :-)

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e537243fed2613487c724651f0b7e7473ed83eb

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ec4e403b9d
Allow to inline UnsafeGetReservedSlot when the object is typed as MIRType::Value. r=jandem

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1ec4e403b9d