1128157 - Getters don't get inlined in a testcase involving nodelists

Status: RESOLVED DUPLICATE of bug 965992

(Core :: JavaScript Engine: JIT, defect)

Description

[Michał Wadas]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140830211128

Steps to reproduce:

I have compared cost of reading `.length` for following objects:

var boostedNodeList = document.querySelectorAll('#wow > span');
(function(){
var n = boostedNodeList.length;
Object.defineProperty(boostedNodeList, 'length', {
get: function(){return n;}
})
}());
var defaultNodeList = document.querySelectorAll('#wow > span');

Related jsperf - http://jsperf.com/nodelistoptimisation/2


Actual results:

reading `bostedNodeList.length` is 10 times slower than reading `defaultNodeList.length`

It's also **1000 times slower** than the same code in Chromium.


Expected results:

Reading `bostedNodeList.length` should be as fast as directly reading `n` variable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Related jsperf - http://jsperf.com/nodelistoptimisation/2

On this page, I see the "boostedNodeList" case also much slower than the other one in Chrome, for what it's worth.

At first glance what things look like to me is that the function is in fact compiled into a constant return value.  However the .length on the nodelist ends up using an inline cache to call the function, again, afaict, which is not all that cheap...

Comment 2

[Boris Zbarsky [:bzbarsky]]

In any case, what's happening here is that since nodelist has to be a proxy the "length" getter here is defined on the expando object, not the nodelist directly.  So the baseline IC stub is of type GetProp_DOMProxyShadowed, which isn't a type that BaselineInspector::commonGetPropFunction knows about, so IonBuilder::getPropTryCommonGetter does nothing useful and we end up falling through to an Ion IC.

We _could_ try to do something better for non-overridebuiltins DOM proxies for which we discover that they're shadowing, because that means the property lives on the expando object and we can in fact try to do something smart with that...  But we'd need to either add some shape guards on the expando object or something (e.g. in this testcase the property is non-configurable so we might be able to eliminate the shape guards).

Comment 3

[Michał Wadas]

DOM performance is ridiculosly slow in all browsers. I think it can be improved by directly creating .length property on immutable collections (like NodeList returned by querySelectorAll) and omitting even getter. 
It can potetially break only (?) hasOwnProperty and getOwnPropertyDescriptor (but it was never guaranted to work consistently across browsers).

Comment 4

[Boris Zbarsky [:bzbarsky]]

And just to prove that the issue here is in fact the nodelist proxy, compare http://jsperf.com/nodelistoptimisation/11 where both we and Chrome do way better on the "boosted" version (though it's still not nearly as fast in Firefox as an actual constant value would be, afaict; compare <http://jsperf.com/nodelistoptimisation/12>).

> I think it can be improved by directly creating .length property on immutable collections

That wouldn't be any better in this case; it would still end up going through the proxy machinery.  There's literally no place on the actual NodeList object to store the property.

What would actually make things sane for the immutable-collections case is to stop the NodeList insanity and return an Array subclass.  That's what callers typically want anyway.  This is what the updated query APIs do, but Array subclassing is not supported very well in JS engines yet, so those APIs haven't been implemented yet.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> compare http://jsperf.com/nodelistoptimisation/11

This was buggy.  The right thing is http://jsperf.com/nodelistoptimisation/13 which shows the getter in fact not getting inlined.  

Filed bug 1128646 on the obvious issue Jan noticed there, but fixing that will not be sufficient for nodelists.

Comment 6

[Jan de Mooij [:jandem]]

Unfortunately the jsperf tests here have disappeared :( I wanted to see how bug 1128646 affected things...

Comment 7

[Michał Wadas]

Following code can be run in console:


var boostedNodeList = document.querySelectorAll('*');
(function(){
var n = boostedNodeList.length;
Object.defineProperty(boostedNodeList, 'length', {
get: function(){return n;}
})
}());
var defaultNodeList = document.querySelectorAll('*');
console.time('Normal node list');
for(var j=0; j < 100; j++) for(var i=0; i < defaultNodeList.length; i++);
console.timeEnd('Normal node list')

console.time('Getter node list');
for(var j=0; j < 100; j++) for(var i=0; i < boostedNodeList.length; i++);
console.timeEnd('Getter node list')


Now it seems to have similar time (FF36).

Comment 8

[Boris Zbarsky [:bzbarsky]]

The test in comment 7 is quite different from the original jsperf testcases because it doesn't run enough iterations to actually end up in the JIT fast paths.  But yes, doing something like that with a much higher iteration count (or just recreating the jsperf; I wonder why that went away anyway) would work.

Comment 9

[Michał Wadas]

http://jsperf.com/js-getter-as-perfomance-boost

Comment 10

[Boris Zbarsky [:bzbarsky]]

Thanks.

As expected, not much change, since the new ICs know nothing about DOM proxy expando objects...

Comment 11

[Jan de Mooij [:jandem]]

Clearing needinfo; I'm working on other (perf) issues right now. Maybe we can get to this later this year.

Comment 12

[Jan de Mooij [:jandem]]

See bug 965992 comment 7 :)

Comment 13

[Tom S. (please needinfo tschuster)]

\o/