Last Comment Bug 812333 - Replace Node quickstubs with new binding methods
: Replace Node quickstubs with new binding methods
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on: 773780 814022 814821 822691 833518
Blocks: 362571
  Show dependency treegraph
 
Reported: 2012-11-15 13:49 PST by Boris Zbarsky [:bz]
Modified: 2013-01-22 20:36 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make WrapNewBindingObject work with nsIHTMLCollection (3.51 KB, patch)
2012-11-21 08:23 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Review
v1 (60.57 KB, patch)
2012-11-21 08:36 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-11-15 13:49:57 PST
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 [:bz] 2012-11-15 18:30:30 PST
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 [:bz] 2012-11-15 23:35:04 PST
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 [:bz] 2012-11-20 11:13:11 PST
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] 2012-11-21 08:23:03 PST
Created attachment 684031 [details] [diff] [review]
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 [:bz] 2012-11-21 08:31:27 PST
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] 2012-11-21 08:36:08 PST
Created attachment 684040 [details] [diff] [review]
v1

Green on try with attachment 684031 [details] [diff] [review] and the patch from bug 814022.
Comment 7 Boris Zbarsky [:bz] 2012-11-21 21:23:26 PST
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 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-11-22 10:37:50 PST
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 <peterv@propagandism.org> - 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 <peterv@propagandism.org> - 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 <peterv@propagandism.org> - 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
dev-tree-management@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-tree-management
Comment 10 Boris Zbarsky [:bz] 2012-11-22 16:26:22 PST
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 [:bz] 2012-11-22 16:26:52 PST
Oh, http://perf.snarkfest.net/compare-talos/index.html?oldRevs=2a9472ee312c&newRev=065f7c04346e&submit=true is the full compare-talos for this change.
Comment 13 Boris Zbarsky [:bz] 2012-11-22 20:48:41 PST
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 [:bz] 2012-11-23 20:24:51 PST
Filed bug 814821 on the dromaeo regression.

Note You need to log in before you can comment on or make changes to this bug.