894447 - Regression on ss-tagcloud from not immediately deoptimizing typesets on string SETELEM (lots more Ion bailouts)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 893897 caused a regression on ss-tagcloud.  bhackett thinks the issue is the parseJSON bit that does v[i] = n.

Profiling this is a PITA but at first glance it looks like we end up with Ion bailouts now when we didn't use to before.  A simple test confirms it.  Without the patch for bug 893897:

  env IONFLAGS=bailouts ../obj-firefox/dist/bin/js /tmp/test.js |& wc -l
       1

with the patch:

  env IONFLAGS=bailouts ../obj-firefox/dist/bin/js /tmp/test.js |& wc -l
    2252

Comment 1

[Boris Zbarsky [:bzbarsky]]

The bailouts are happening for "function walk".  

The typical bailout looks like this:

[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=149,line=193,uses=424,stubs=0): SetElem(setelem)
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=75,line=190,uses=425,stubs=0): NewArray
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=88,line=190,uses=425,stubs=0): Call(funapply)
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=149,line=193,uses=425,stubs=0): SetElem(setelem)
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=75,line=190,uses=426,stubs=0): NewArray
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=88,line=190,uses=426,stubs=0): Call(funapply)
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=159,line=193,uses=427,stubs=0): IteratorClose
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=149,line=193,uses=427,stubs=0): SetElem(setelem)
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=75,line=190,uses=428,stubs=0): NewArray
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=88,line=190,uses=428,stubs=0): Call(funapply)
[BaselineICFallback] Fallback hit for (/tmp/test.js:185) (pc=117,line=191,uses=428,stubs=2): GetElem(getelem)
[Bailouts] Took bailout! Snapshot offset: 461

over and over and over again.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, I got that by just putting http://mxr.mozilla.org/mozilla-central/source/js/src/devtools/jint/sunspider/string-tagcloud.js?force=1 in /tmp/test.js and then running shell on it...

We need to figure out what's causing all the bailouts.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We also have a regression on the dromaeo test that tests assigning expandos to DOM objects; somewhere on the order of a 20% hit on that subtest, translating to a 2-5% hit on dromaeo-dom in general.  

I'm not sure why that's happening, but seems pretty related to this bug...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, http://perf.snarkfest.net/compare-talos/index.html?oldRevs=d9a84be1a35c&newRev=0e32217fa899&submit=true for the compare-talos.

Comment 5

[Jan de Mooij [:jandem]]

The bailouts on tagcloud are from a MMonitorTypes instruction we add for |n| here, to check it's included in the LHS property TypeSet:

v[i] = n;

Two problems I noticed at a first glance:

(1) We shouldn't add a MonitorTypes followed by a CallSetElement -- the VM call will handle the monitoring etc.

(2) We add a freeze constraint for the property type but somehow this doesn't trigger recompilation.

Looking into (2) now.

Comment 6

[Jan de Mooij [:jandem]]

Ah, so the problem is that IonBuilder::jsop_setelem calls PropertyWriteNeedsTypeBarrier with PropertyName == NULL (treated as JSID_VOID). However, v[i] = n does not access an indexed property but a named one (i is a string) and we will always bailout...

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Fixes both problems in comment 5 by calling ElementAccessIsDenseNative before PropertyWriteNeedsTypeBarrier.

The extra object->resultTypeSet() NULL check is necessary because MGuardObjectType does not have a result TypeSet.

Should I also revert/backout bug 894463?

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 776729 [details] [diff] [review]
Patch

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

Yeah, if this fixes the ss regression then the workaround in bug 894463 should be backed out.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So for what it's worth, the workaround in bug 894463 fixed the dromaeo-css regression but not the dromaeo-dom, as expected.  Going to test this patch on dromaeo-dom.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Seems to help, yes.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Actually, no.  Seems to have no effect on dromaeo-dom (in particular, on dom-attr, which is what I've been testing)

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Actually, no.  Seems to have no effect on dromaeo-dom (in particular, on
> dom-attr, which is what I've been testing)

It's possible there's a similar problem somewhere else. Do you have a small shell testcase?

Comment 13

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aed60373019

(Linux64 Try: https://tbpl.mozilla.org/?tree=Try&rev=8b8e9417b6ed)

Also reverts bug 894463.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Do you have a small shell testcase?

It's hard to create a small shell testcase for DOM nodes.  ;)

The dromaeo test in question ends up looking more or less like this, in browser:

  var a = document.getElementsByTagName("a")[0];
  var num = 10240;
  var f = function()  {
    for ( var i = 0; i < num; i++ )
    a["test" + num] = function(){};
  }
  var start = new Date;
  var count = 0;
  while (new Date - start < 1000) {
    ++count;
    f();
  }
  alert(count/(new Date - start) * 1000);

More or less...  The f in question is actually a closure, not just a global function.

If I do a shell testcase like this, though (using an object literal for "a"), I can't even reproduce a slowdown from bug 893897.  :(

Comment 15

[Boris Zbarsky [:bzbarsky]]

Should I just file a separate bug on the dromaeo-dom thing?

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Should I just file a separate bug on the dromaeo-dom thing?

Yeah that would be good.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, filed bug 894921.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4aed60373019