Closed Bug 812333 Opened 12 years ago Closed 12 years ago

Replace Node quickstubs with new binding methods

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: peterv)

References

Details

Attachments

(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.
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: nobody → peterv
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Consider turning on WebIDL Node binding for all nodes → Replace Node quickstubs with new binding methods
Status: REOPENED → ASSIGNED
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? r=me
Attachment #684031 - Flags: review?(bzbarsky) → review+
Depends on: 814022
Attached patch v1 (deleted) — Splinter 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] 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+
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
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. :(
Status: ASSIGNED → RESOLVED
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: 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.
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.

Attachment

General

Created:
Updated:
Size: