Closed
Bug 812333
Opened 12 years ago
Closed 12 years ago
Replace Node quickstubs with new binding methods
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bzbarsky, Assigned: peterv)
References
Details
Attachments
(2 files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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? :(
Reporter | ||
Comment 2•12 years ago
|
||
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).
Reporter | ||
Comment 3•12 years ago
|
||
Wontfixing this, per discussion with Peter. The Xray bit would be hell, and just not worth it for a temporary situation.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Summary: Consider turning on WebIDL Node binding → Consider turning on WebIDL Node binding for all nodes
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → peterv
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Consider turning on WebIDL Node binding for all nodes → Replace Node quickstubs with new binding methods
Assignee | ||
Updated•12 years ago
|
Status: REOPENED → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 4•12 years ago
|
||
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)
Reporter | ||
Comment 5•12 years ago
|
||
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
Attachment #684031 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #684040 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 7•12 years ago
|
||
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.
Attachment #684040 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/36527dc12585
https://hg.mozilla.org/integration/mozilla-inbound/rev/065f7c04346e
Target Milestone: --- → mozilla20
Comment 9•12 years ago
|
||
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
Reporter | ||
Comment 10•12 years ago
|
||
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. :(
Reporter | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/36527dc12585
https://hg.mozilla.org/mozilla-central/rev/065f7c04346e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 13•12 years ago
|
||
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.
Reporter | ||
Comment 14•12 years ago
|
||
Filed bug 814821 on the dromaeo regression.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•