Per bug #648801 comment #41: Times for test2.html are greater on Firefox 10 than on Firefox 9.0.1: Win 7: Firefox 9: 1258-681, 1253-662, 1240-639, 1258-630,1273-673 Firefox 10: 1361-1032, 1613-1006, 1647-1006, 1632-1010, 1592-1017 Mac OS X 10.6: Firefox 9: 1493-827, 1459-828, 1527-821, 1489-812, 1526-816 Firefox 10: 2181-1319, 2092-1335, 2159-1358, 2081-1360, 2135-1314 Linux 11.10: Firefox 9: 1532-757, 1398-754, 1395-825, 1384-772, 1388-764 Firefox 10: 1888-1173, 1780-1143, 1812-1141, 1781-1143, 1830-1141 Mozilla/5.0 (X11; Linux i686; rv:10.0) Gecko/20100101 Firefox/10.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0 Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0 Mozilla/5.0 (Windows NT 6.1; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 Mozilla/5.0 (X11; Linux i686; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0.1) Gecko/20100101 Firefox/9.0.1

So this test is for .item(). I can totally believe this is slower with the proxy-based list bindings, since we no longer get a useful IC. But I thought the regression was much smaller than this... In any case, the profile is pretty similar for both the first and second number. Looking at just the second one: 8% mjit-generated code 27% under item() itself (10% self time, largely inlined Wrap I think, 5% instanceIsListObject, 4% getListObject) 5% self time in stubs::GetProp 19% self time in js::GetPropertyOperation 8% under js::PropertyCache::fullTest 7% self time in proxy_GetGeneric 20% under ListBase::get() (7% self time, 4% getListObject, 8% self time in getPropertyOnPrototype). So about half the time is in the nodelist proxy (and maybe we can make this faster). The other half is mostly the "JM is not very good at optimizing proxies" bit. The fullTest part is silly for a proxy, for example. The stubcall is costing us (e.g. the time in GetPropertyOperation in large part function entry/exit overhead (and a good chunk of the rest is the obj->getOps()->getProperty null-check). I _think_ we have existing bugs on better codegen for proxies; this one should depend on those.

Bug 717009 will help with some of the nodelist proxy internals.

Ah, bug 649887 is the bug covering the JS engine bits here.

So at this point, the second number in the test looks pretty good. But the first number is awful. At least 4x slower than it was in fx9/fx10! The regression seems to be when ion landed....

Sean Stangl did some digging into this. The reason the first number is so terrible is that each nodelist has a different shape, so we just blow out our ListBase IC and fail. And the reason each nodelist has a different shape is that shape covers _parent_ and the parent of a childNodes is the node it was gotten from. So that really sucks; it means that our parenting setup is a perf hit in many cases. I manually hacked nsChildContentList to use the OwnerDoc() of the element for the parent, and my times dropped by almost 10x. I think it's pretty easy to change our parenting setup so everything that's not a node is always just parented to the global. Will that be ok? The obvious worry is RescueOrphans, but if we're always parented to things that can't get reparented, do we care?

(In reply to Boris Zbarsky (:bz) from comment #5) > I think it's pretty easy to change our parenting setup so everything that's > not a node is always just parented to the global. Will that be ok? The > obvious worry is RescueOrphans, but if we're always parented to things that > can't get reparented, do we care? Well, the traditional reason for a lot of this funny parenting is that the child object needs some way to access the parent object, so we let the parent slot do double-duty of scope stuff and being a reference to an object we care about. We could add a separate reserved slot for that stuff, but that increases overhead as long as we still have the parent slot, which in turn we can't get rid of until the DOM stops using it. Chicken-and-egg. My big concern with the patch as-attached is that it will break assumptions in the nodelist code that use it to access certain nodes. But that's not my area of expertise. As far as wrapper reparenting, we don't really care. Wrapper reparenting only matters for XPCWNs, and nodes that are actually in a document tree. So we just punt for the former with the new bindings, and handle the latter in CloneAndAdopt.

> the child object needs some way to access the parent object I don't think we do that anywhere; I just checked JS_GetParent and js::GetObjectParent callsites....

(In reply to Boris Zbarsky (:bz) from comment #8) > I don't think we do that anywhere; I just checked JS_GetParent and > js::GetObjectParent callsites.... In that case, I'm all for it! This will be much easier than I thought.

Is the scope chain thing here an issue? The JS engine still uses getParent() for lexical lookups on non-global, non-internal-scope objects so if these objects appeared on scope chains anywhere then behavior would change. If this is an issue, the engine could either be taught about these objects and how to find the enclosing scope, or a class hook could be added which gets the enclosing scope however it chooses.

> Is the scope chain thing here an issue? Only for elements, which is why I excluded nodes in my patch (though maybe I should just exclude elements). But yes, if we can do the scope chain in some other way, we could just always parent everything to the global here and be done with it.

(In reply to Brian Hackett (:bhackett) from comment #10) > Is the scope chain thing here an issue? The JS engine still uses > getParent() for lexical lookups on non-global, non-internal-scope objects so > if these objects appeared on scope chains anywhere then behavior would > change. If this is an issue, the engine could either be taught about these > objects and how to find the enclosing scope, or a class hook could be added > which gets the enclosing scope however it chooses. Yes! Now that we've determined that nobody needs their parent to find relevant data, the scope thing is the last remaining reason we need parents for the DOM. If you can make an alternative way to do that in the JS engine, I'll hook it up to the DOM and will can kill parents entirely!

> Now that we've determined that nobody needs their parent to find > relevant data, the scope thing is the last remaining reason we need parents > for the DOM. If you can make an alternative way to do that in the JS engine, > I'll hook it up to the DOM and will can kill parents entirely! Relevant: bug 805052 is about removing object parents entirely.

The only downside I can see is that I do think we'll eventually want to move stuff hanging off of nodes with the node if it gets adopted into a different compartment. Having the node be the parent makes it easier to find the binding objects that need to be moved.

(In reply to Peter Van der Beken [:peterv] from comment #14) > The only downside I can see is that I do think we'll eventually want to move > stuff hanging off of nodes with the node if it gets adopted into a different > compartment. Having the node be the parent makes it easier to find the > binding objects that need to be moved. Yeah, but given the direction of the edge, we'd need to do some kind of whole-compartment sweep anyway, right? If/when we do that, it'll probably end up having to be some kind of hash-map keyed by nodes, I think. Oh hashmaps... :-(

Comment on attachment 733677 [details] [diff] [review] Stop using non-global parent objects for everything that's not a node, since having a mix of parent objects makes us fail shape guards in the JIT. Review of attachment 733677 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ +1058,5 @@ > + js::GetGlobalForObjectCrossCompartment(aParentObject) : nullptr; > +} > + > +static inline JSObject* > +GetRealParentObject(nsINode* aParent, JSObject* aParentObject) Let's do Element.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a56da67edc This is now So Fixed. ;)