Closed Bug 812333 Opened 12 years ago Closed 12 years ago

Replace Node quickstubs with new binding methods


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

Not set





(Reporter: bzbarsky, Assigned: peterv)




(2 files)

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.
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?  :(
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).
Wontfixing this, per discussion with Peter.  The Xray bit would be hell, and just not worth it for a temporary situation.
Closed: 12 years ago
Resolution: --- → WONTFIX
Summary: Consider turning on WebIDL Node binding → Consider turning on WebIDL Node binding for all nodes
Assignee: nobody → peterv
Resolution: WONTFIX → ---
Summary: Consider turning on WebIDL Node binding for all nodes → Replace Node quickstubs with new binding methods
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #684031 - Flags: review?(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?

Attachment #684031 - Flags: review?(bzbarsky) → review+
Depends on: 814022
Attached patch v1Splinter Review
Green on try with attachment 684031 [details] [diff] [review] and the patch from bug 814022.
Attachment #684040 - Flags: review?(bzbarsky)
Comment on attachment 684040 [details] [diff] [review]

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.
Attachment #684040 - Flags: review?(bzbarsky) → review+
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   :

Changeset range:

    : Peter Van der Beken <> - Fix for bug 814022 (Make instanceof for new DOM bindings work across scopes). r=bz.

    : Peter Van der Beken <> - Fix for bug 812333 (Replace Node quickstubs with new binding methods) - part 1: make WrapNewBindingObject asserts deal with HTMLCollection. r=bz.

    : Peter Van der Beken <> - Fix for bug 812333 (Replace Node quickstubs with new binding methods) - part 2: install Node's binding methods on the XPConnect proto. r=bz.

  * - Replace Node quickstubs with new binding methods
  * - Fix instanceof for new DOM bindings
dev-tree-management mailing list
Hmm.  Looking at 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.  :(
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
So a Mac dromaeo run comparison with before on the left and after on the right shows no significant change:,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 (pre-change) and (post-change) for a month or so.  Will look at things on Linux on Monday.
Filed bug 814821 on the dromaeo regression.
Depends on: 814821
Blocks: 362571
Depends on: 773780
Depends on: 822691
Depends on: 833518
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.