1086530 - Huge regression with using split after landing of bug 1054330

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

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

Comment 1

[Nicolas B. Pierron [:nbp]]

[Tracking Requested - why for this release]:
Performance regression, which should have a small fix.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Victor Carlquist]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I can confirm that without he patch for bug 688219 I see the testcase in comment 0 take

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Kannan Vijayan [:djvj]]

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.

Comment 7

[Victor Carlquist]

Attachment: bug1086530

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

Comment 8

[Kannan Vijayan [:djvj]]

Comment on attachment 8509149 [details] [diff] [review]
bug1086530

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

Thanks for writing that!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb4c2888a1

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3350864&repo=mozilla-inbound

Comment 11

[Victor Carlquist]

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!

Comment 12

[Victor Carlquist]

Attachment: bug1086530fix.patch

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.

Comment 13

[Kannan Vijayan [:djvj]]

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.

Comment 14

[Kannan Vijayan [:djvj]]

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.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3922ae1c9aa7

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3922ae1c9aa7

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3922ae1c9aa7

Comment 18

[Boris Zbarsky [:bzbarsky]]

Please don't forget to request aurora approval.

Comment 19

[Victor Carlquist]

(In reply to Boris Zbarsky [:bz] from comment #18)
> Please don't forget to request aurora approval.

Thanks Boris!

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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]:

Comment 21

[Victor Carlquist]

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.

Comment 22

[Victor Carlquist]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #20)

Thanks Lukas.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Thank you - approving uplift.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/151b508552c4