Closed Bug 1086530 Opened 7 years ago Closed 7 years ago

Huge regression with using split after landing of bug 1054330

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 + fixed

People

(Reporter: h4writer, Assigned: victorcarlquist)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1054330 caused a big slowdown in a particular testcase. I suspect we cannot DCE something now:

> var matcher = /e/g;
> var testString = "We the People of the United States, in Order to form a more perfect Union, establish Justice, insure domestic Tranquility, provide for the common defence,promote the general Welfare, and secure the Blessings of Liberty to ourselves and our Posterity, do ordain and establish this Constitution for the United States of America.";
> 
> 
> function test(testString) {
>     return testString.split("e").length - 1;
> }
> 
> for(var i=0; i<1000000; i++) {
>     test(testString);
> }

Before bug 1054330:

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-inbound/js/src/build-32-parallel$ time js /tmp/test43.js
real    0m0.020s
user    0m0.016s
sys     0m0.004s

After bug 1054330:

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-inbound/js/src/build-32-parallel$ time js /tmp/test43.js
real    0m1.870s
user    0m1.884s
sys     0m0.164s
Blocks: 1054330
Flags: needinfo?(kvijayan)
[Tracking Requested - why for this release]:
Performance regression, which should have a small fix.
I bet what happens is that we end up in IonBuilder::inlineStringSplit, call this:

  JSObject *templateObject = inspector->getTemplateObjectForNative(pc, js::str_split);

and end up in BaselineInspector::getTemplateObjectForNative which walks the IC stubs looking for stubs that are isCall_Native().

But now we have a ICCall_StringSplit stub, so we return null and end up not inlining split() in ion (and yes, hence no DCE and so forth).
If the issue is in IonBuilder::inlineStringSplit, so when the bug688219 is landed, this regression will be fixed, because we'll use the ICCall_StringSplit's templateObject to copy and return it on Ion.
I can confirm that without he patch for bug 688219 I see the testcase in comment 0 take
Er, takes 1.79s, while after applying that patch and sprinkling as<NativeObject>() to make it build it takes 0.1s.

Are we planning to backport bug 688219 to 35?  If not, we may want a targeted fix here anyway.
Ouch.  That is unfortunate.  As Victor mentioned this will be resolved once the ion optimization corollary for this lands.

However, the quick fix for this is to have BaselineInspector::getTemplateObjectForNative directly:

diff --git a/js/src/jit/BaselineInspector.cpp b/js/src/jit/BaselineInspector.cpp
--- a/js/src/jit/BaselineInspector.cpp
+++ b/js/src/jit/BaselineInspector.cpp
@@ -445,16 +445,18 @@ BaselineInspector::getTemplateObjectForN
 {
     if (!hasBaselineScript())
         return nullptr;

     const ICEntry &entry = icEntryFromPC(pc);
     for (ICStub *stub = entry.firstStub(); stub; stub = stub->next()) {
         if (stub->isCall_Native() && stub->toCall_Native()->callee()->native() == native)
             return stub->toCall_Native()->templateObject();
+        if (stub->isCall_StringSplit() && native == js::str_split)
+            return stub->toCall_StringSplit()->templateObject();
     }

     return nullptr;
 }

 DeclEnvObject *
 BaselineInspector::templateDeclEnvObject()
 {


Victor: can you check if something like the above fixes the issue?  I think the diff above as-is won't be enough, since |BaselineInspector::getTemplateObjectForNative| has to return a NativeObject*, and the templateObject field on the stringsplit stub is a HeapPtrObject (not HeapPtrNativeObject).  This might need to be fixed.
Flags: needinfo?(kvijayan)
Attached patch bug1086530 (obsolete) — Splinter Review
Yeah, It's ok now.

before
-------------------
real	0m3.150s
user	0m3.175s
sys	0m0.216s
-------------------

after
-------------------
real	0m0.030s
user	0m0.024s
sys	0m0.009s
Attachment #8509149 - Flags: review?(kvijayan)
Comment on attachment 8509149 [details] [diff] [review]
bug1086530

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

Thanks for writing that!
Attachment #8509149 - Flags: review?(kvijayan) → review+
Attachment #8509149 - Flags: checkin?
Attachment #8509149 - Flags: checkin? → checkin+
sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3350864&repo=mozilla-inbound
Flags: needinfo?(victorcarlquist)
hmmm, I'll investigate it (:

I tested it on my PC (Linux x64), but I couldn't reproduce this failure with "--ion-eager --baseline-eager" params.
I'll clone the mozilla-inbound to test it.

I have a question: Why in the link above is linking to revision=86d5e68991b2 ?

Thanks Carsten!
Flags: needinfo?(victorcarlquist)
Hi!
Sorry for the delay...
The problem was that |templateObject| was nursery, so, nursery can't be used in MConstant on MStringSplit.

I changed the following lines at BaseliseIC.cpp:
 
> -    RootedArrayObject newObj(cx, NewDenseArray(cx, length, type, NewArray_FullyAllocating));
> +    RootedArrayObject newObj(cx, NewDenseFullyAllocatedArray(cx, length, nullptr, TenuredObject));

I think that will solve the failure.

Thanks.
Attachment #8509149 - Attachment is obsolete: true
Attachment #8514939 - Flags: review?(kvijayan)
Comment on attachment 8514939 [details] [diff] [review]
bug1086530fix.patch

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

Sorry for the delay.  Was on PTO from Thursday of last week.  Looks good. I'm going to try-run it before setting checkin-needed this time.
Attachment #8514939 - Flags: review?(kvijayan) → review+
Comment on attachment 8514939 [details] [diff] [review]
bug1086530fix.patch

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

Try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=d16fb8ff297d

Setting checkin-needed again.
Attachment #8514939 - Flags: checkin?
Attachment #8514939 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/3922ae1c9aa7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please don't forget to request aurora approval.
Flags: needinfo?(victorcarlquist)
Attachment #8514939 - Flags: approval-mozilla-aurora?
(In reply to Boris Zbarsky [:bz] from comment #18)
> Please don't forget to request aurora approval.

Thanks Boris!
Flags: needinfo?(victorcarlquist)
Victor, please fill out an approval request comment so we have more information to go on:

Approval Request Comment
[Feature/regressing bug #]: 
[User impact if declined]: 
[Describe test coverage new/current, TBPL]: 
[Risks and why]:
[String/UUID change made/needed]:
Flags: needinfo?(victorcarlquist)
Comment on attachment 8514939 [details] [diff] [review]
bug1086530fix.patch

Approval Request Comment
[Feature/regressing bug #]: 1054330
[User impact if declined]: Huge regression (~100x slower) on split function.
[Describe test coverage new/current, TBPL]:  Some local tests.
[Risks and why]: Low risk because the code returns a valid templateObject for StringSplit in Ion.
[String/UUID change made/needed]: none.
Flags: needinfo?(victorcarlquist)
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #20)

Thanks Lukas.
Comment on attachment 8514939 [details] [diff] [review]
bug1086530fix.patch

Thank you - approving uplift.
Attachment #8514939 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.