Mihaela Velimiroviciu (:mihaelav) Reporter
	Description • 13 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 13 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 13 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 11 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 11 years ago

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?

	Bobby Holley (:bholley)
	Comment 7 • 11 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 11 years ago

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

	Bobby Holley (:bholley)
	Comment 9 • 11 years ago

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

	Brian Hackett [Laid off!]
	Comment 10 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 11 years ago

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

	Bobby Holley (:bholley)
	Comment 12 • 11 years ago

(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!

	Nicholas Nethercote [inactive]
	Comment 13 • 11 years ago

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

	Peter Van der Beken [:peterv]
	Comment 14 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 15 • 11 years ago

(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... :-(

	Peter Van der Beken [:peterv]
	Comment 16 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 11 years ago

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

This is now So Fixed.  ;)

	Ed Morley [:emorley]
	Comment 18 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/f5a56da67edc