Closed Bug 683631 Opened 9 years ago Closed 7 years ago

JM+TI: Dromaeo DOM and CSS regressions

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox9 - wontfix

People

(Reporter: bzbarsky, Assigned: bhackett1024)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: js-triage-needed)

The Dromaeo DOM and CSS tests seem to have regressed with TI, and the regression doesn't go away when bug 663138 is fixed.

http://dromaeo.com/?id=148582,148576 shows some numbers (left column is pre-TI, right column is post-TI with patch for bug 663138), which includes some of the above tests as well as unrelated string/array tests.  There's a particularly noticeable regression on the jquery numbers, and also for expandos on nodes.

Requesting tracking for mozilla9 on this, because this is code that sites actually use.
Let's start by finding out why this happens. I don't think it necessarily needs tracking if we don't have more evidence that it will harm user experience.
Assignee: general → bhackett1024
Whiteboard: js-triage-needed
For what it's worth, I consider "lots of jquery operations are slower" to be evidence that it will harm the user experience....

But yes, I agree that understanding the source of the slowdown is the priority here.
Setting tracking to make it clear that there is action required here, which is initially to understand the cause.
Summary: Dromaeo DOM and CSS regressions from TI landing → JM+TI: Dromaeo DOM and CSS regressions
I got -D working in the browser and have been using that with the CSS jQuery tests to look at this.  Short version: it looks like we make a *lot* of DOM-related stubcalls, with or without TI, and this regression is primarily due to the extra overhead associated with these calls.

The Dromaeo CSS jQuery tests just run a bunch of jQuery() calls over and over on a document.  jQuery is a function for doing CSS-style queries to fetch particular elements from a document while in JS.  Stuffing these queries in a loop and running them over and over puts TI at about 12% slower than Fx6.

Using -D points out hotspots pretty well.  The main issue with these measurements is that -D only accumulates counts of stub calls and instructions taken, and is not timing information.  This is a problem when stub calls take varying amounts of time, and I want to get timing for stubs in so that we know which calls are costing the most, rather than which are taken the most.  Anyways, in the file at http://dromaeo.com/lib/jquery.js:

Guts of the merge() function, line 1154:

while ( elem = second[ i++ ] )
    first[ pos++ ] = elem;

'first' is always a dense array, and writes to it are in jitcode.  25% of the time second is a dense array, 75% of the time it is an HTMLCollection.  The latter case gets stubbed, 12% of the total stubcalls we take.

Guts of the classFilter() function, line 1656:

for ( var i = 0; r[i]; i++ ) {
    var pass = (" " + r[i].className + " ").indexOf( m ) >= 0;
    if ( !not && pass || not && !pass )
        tmp.push( r[i] );
}

'r' is always a dense array, containing DOM elements and strings.  We take calls for the .className, both string adds, the indexOf (unavoidable), and the !not (not is undefined).  24% of the total stubcalls.

Start of the data() function, line 657:

data: function( elem, name, data ) {
    elem = elem == window ?
        windowData :
        elem;

    var id = elem[ expando ];
    ...
}

No loop, but this function is called a lot, with elem a DOM element.  We mainly take stubs for a FixupArity at the head (I think), the 'elem == window' (class equality hook?) and the elem[expando].  expando is a fixed string.  9% of total stubs.

Loop deep in the filter() function, line 1740:

for ( var n = parentNode.firstChild; n; n = n.nextSibling )
    if ( n.nodeType == 1 )
        n.nodeIndex = c++;

This loop seems pretty hot.  n is a DOM element, the nodeIndex write is an expando property.  All property accesses are stubbed, about 10% of total stubs.

It goes on like this.

Broadly, looking at this I think that the extra TI call overhead has taken some stuff that used to be pretty slow and made it a little slower.  Finding band-aids to make us less slow is one option, but imo the better way forward is to direct energy at making this stuff FAST, which means doing it all in jitcode.  This would be a longer timeframe than Fx9, clearly, but maybe not that much longer.  I need to understand the DOM better.  This would not be effort duplicated for IonMonkey either, as most of the work is in making sure the necessary type information and VM invariants are in place that will allow second[i] or r[i].className or n.nextSibling to be a clean and uncluttered compiler path.
> it looks like we make a *lot* of DOM-related stubcalls

Yep.  See bug 557358 and its various dependencies (both depends on and blocks).

Basically, almost every single DOM operation ends up as a stubcall in JM, with no obvious plans to change that.  We raised this as the #1 performance concern from the DOM side at the last onsite... and then nothing happened.  :(

The HTMLCollection thing is about to become a proxy, but that won't help with the stubcalls issue, right?

DOM objects do have a class equality hook, sadly.  I wonder whether we can nuke that and just have the hook on the various wrappers.  Ccing Andreas and Blake for that part.

I agree that the right solution is to stop stubcalling all this stuff...  How much work would it be to at least wrap up bug 557358?

In any case, the bad news from the above analysis is that it sounds like basically DOM performance did in fact regress 10-15% across the board, since it's all stubcalled.  Do we see that as an acceptable tradeoff?  I have to admit I'm having a hard time seeing it that way... but that may be my general frustration with the way this known problem has been ignored for a year or so.
Depends on: 684305
(In reply to Boris Zbarsky (:bz) from comment #5)
> I have to admit I'm having a hard time seeing it that way...

Same here.
(In reply to Boris Zbarsky (:bz) from comment #5)
> DOM objects do have a class equality hook, sadly.  I wonder whether we can
> nuke that and just have the hook on the various wrappers.  Ccing Andreas and
> Blake for that part.

I don't think that the equality hook for DOM objects is necessary.
Well, an equality hook _somewhere_ is needed to make == between objects and various security wrappers for them work, right?  I thought that was the point of the hook....
No, we actually no longer need the equality hook (*). Compartments are membranes and enforce that you  only have a single view of an object. If its same compartment, you can only get your hands on the direct (DOM) object. If its in a different compartment, you can only get your hands on one wrapper for it, and you can never touch the direct object. We should be able to nuke the equality hook.

(*) .wrappedJSObject used to compare true with the object itself. We already break that anyway I think.
Actually, I think the existing equality hooks track back to an xpconnect issue. We can get different WNs for some natives (one for each scope), and those will not compare equal. However, we can use the new compartment-per-global stuff to eliminate WN scopes and use the comapartment for that. This will probably have some addon compatibility implications. Not Fx9 stuff for sure.
> We can get different WNs for some natives

Yes, but not for the ones where DOM perf really matters.

I filed bug 684435 on nuking them ASAP where we can.
Depends on: 684435
Depends on: 557358
Depends on: 622302
OK, it looks like these changes aren't gonna happen for Fx9. This is blocking infer-perf-regress, so it shouldn't get lost.
We did land bug 684435 for Fx9.  Not sure whether bug 557358 landed there or not; it's missing a target milestone setting...
Oh, I meant to update this bug.  When aurora recently merged to beta dev-tree-management saw improvements in all Dromaeo tests except jslib, which is not I believe testing DOM stuff (and was improved by some patches for Fx10 like array.concat speedup).  There were DOM changes that went into Fx9 which also sped up DOM stuff, but IIRC I compared pre-TI Dromaeo and post-TI plus getter PICs and a few other fixes that went in at the same time, and CSS/DOM were either flat or slight improvements.  So I think this one if fixed.
(In reply to Boris Zbarsky (:bz) from comment #13)
> We did land bug 684435 for Fx9.  Not sure whether bug 557358 landed there or
> not; it's missing a target milestone setting...

Bug 667358 landed in time for Fx9, I just updated that bug.
(In reply to Brian Hackett from comment #15)
> Bug 667358 landed in time for Fx9, I just updated that bug.

Argh, this should be bug 557358.
Closing based on comment 14
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.