812333 - Replace Node quickstubs with new binding methods

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We can do this even though no nodes are converted.  As nodes are converted, those nodes will run faster.

I tried this locally, and at least firstChild performance is within 5% or so of quickstubs, after a bit more inlining pixie dust. So it wouldn't be too much of a perf hit.  I still think we should wait until the next uplift, though.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So this almost works, with all the nsIDOMNode methods marked noscript in the IDL.  The one thing I've found so far that breaks looks like Xrays...  Peter, any bright ideas on how to make that work?  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

It might just be that this is not workable (in that we can't force things that are not converted to go through the new-bindings code).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Wontfixing this, per discussion with Peter.  The Xray bit would be hell, and just not worth it for a temporary situation.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Make WrapNewBindingObject work with nsIHTMLCollection

nsIHTMLCollection implementations have a wrappercache, but nsIHTMLCollection doesn't inherit from nsWrapperCache itself. The assert checks for nsISupports, but that's a runtime assert, so we still try to cast to nsWrapperCache. Making it a compile-time assert works.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 684031 [details] [diff] [review]
Make WrapNewBindingObject work with nsIHTMLCollection

Hmm.  I guess we can have eNonDOMObject if we had a non-DOM object in the wrapper cache but had a compartment mismatch so didn't take the early return?  :(  Maybe document that?

r=me

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1

Green on try with attachment 684031 [details] [diff] [review] and the patch from bug 814022.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 684040 [details] [diff] [review]
v1

As discussed, please file a followup to do chrome methods/attributes too.

Perhaps call the new method DefineWebIDLBindingPropertiesOnXPCProto?  We overuse "New" in binding-land.  ;)

>+++ b/dom/bindings/Bindings.conf
>+    'prefable': True,
>+    'castable': False,

Please document that it's marked 'prefable' because some nodes are not on new bindings yet, so the wrapping code for Node return values needs to fall back to XPConnect as needed.  And document that "'castable': False" is needed because we don't support prefable castable arguments, and we have Node arguments.

Love all the known-fail removals.  ;)

>+++ b/dom/webidl/Node.webidl
>+  readonly attribute DOMString? namespaceURI;
>+  readonly attribute DOMString? prefix;
>+  readonly attribute DOMString? localName;

Maybe document that these are here for compat for now and they should become non-nullable if they move to Element?

>+++ b/js/xpconnect/tests/mochitest/test_bug505915.html

We'll have to revisit this test when we do Document, right?  Might be worth filing a bug on that, blocking the WebIDL-for-Document bug.

r=me.  This is awesome.

Comment 8

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36527dc12585
https://hg.mozilla.org/integration/mozilla-inbound/rev/065f7c04346e

Comment 9

[(no longer active)]

Regression :( Dromaeo (DOM) decrease 2.68% on Linux x64 Mozilla-Inbound-Non-PGO
-------------------------------------------------------------------------------
    Previous: avg 189.344 stddev 1.905 of 30 runs up to revision 2a9472ee312c
    New     : avg 184.267 stddev 0.891 of 5 runs since revision 065f7c04346e
    Change  : -5.077 (2.68% / z=2.666)
    Graph   : http://mzl.la/UsgfSi

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a9472ee312c&tochange=065f7c04346e

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/3e9a567ce55c
    : Peter Van der Beken <[e-mail]> - Fix for bug 814022 (Make instanceof for new DOM bindings work across scopes). r=bz.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=814022

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/36527dc12585
    : Peter Van der Beken <[e-mail]> - Fix for bug 812333 (Replace Node quickstubs with new binding methods) - part 1: make WrapNewBindingObject asserts deal with HTMLCollection. r=bz.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=812333

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/065f7c04346e
    : Peter Van der Beken <[e-mail]> - Fix for bug 812333 (Replace Node quickstubs with new binding methods) - part 2: install Node's binding methods on the XPConnect proto. r=bz.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=812333

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=812333 - Replace Node quickstubs with new binding methods
  * http://bugzilla.mozilla.org/show_bug.cgi?id=814022 - Fix instanceof for new DOM bindings
_______________________________________________
dev-tree-management mailing list
[e-mail]
https://lists.mozilla.org/listinfo/dev-tree-management

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Looking at http://perf.snarkfest.net/compare-talos/breakdown.html?oldTestIds=20914863&newTestIds=20916283&testName=dromaeo_dom it looks like most of the hit was in dom_traverse.  I'll take a look on Monday into what exactly is going on there.

A small hit from this is expected, for now, because the Node WebIDL code has to do a bit more work than the quickstubs in cases when the "this" object is an XPConnect object.  Obviously as we transition more and more nodes to WebIDL binding objects that problem will resolve itself.  But the 9% hit on dom_traverse indicates something is likely wrong.  Unfortunately, on Mac we had an improvement(?) on that test, so I might need to dig up a Linux build to reproduce.  :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, http://perf.snarkfest.net/compare-talos/index.html?oldRevs=2a9472ee312c&newRev=065f7c04346e&submit=true is the full compare-talos for this change.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36527dc12585
https://hg.mozilla.org/mozilla-central/rev/065f7c04346e

Comment 13

[Boris Zbarsky [:bzbarsky]]

So a Mac dromaeo run comparison with before on the left and after on the right shows no significant change: http://dromaeo.com/?id=185034,185035

So my bet is on gcc vs clang there, and something perhaps not being inlined when it really should be...

For my reference, I can get the builds at https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2a9472ee312c (pre-change) and https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=065f7c04346e (post-change) for a month or so.  Will look at things on Linux on Monday.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Filed bug 814821 on the dromaeo regression.