Closed
Bug 814821
Opened 12 years ago
Closed 12 years ago
Dromaeo dom-traverse regression from bug 812333
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bzbarsky, Assigned: peterv)
References
Details
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
See bug 812333 comment 9 and following.
On Linux64 I see http://dromaeo.com/?id=185114,185115 (before on left, after on right).
A definite hit, especially on nextSibling and previousSibling.
I tried force-inlining some stuff (patch coming up), but that didn't seem to help.
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
A microbenchmark for .nextSibling shows almost no hit over here for me on Linux64. :( Maybe 2% at most, not the 30% I'm seeing on Dromaeo.
Reporter | ||
Comment 3•12 years ago
|
||
Reporter | ||
Comment 4•12 years ago
|
||
I also tried calling "nsNodeSH::PreserveWrapper(GetNative(wrapper, obj));" right at the top of nsElementSH::PostCreate, and that has no effect. So it's not like We're GCing and having to rewrap, at first glance.
Really need to profile that Dromaeo test at this point. Peter, do you happen to have a Linux profiling setup?
Assignee | ||
Comment 5•12 years ago
|
||
I've also tried a bunch of stuff based on what I saw by profiling on OS X (inlining, using XPConnect fast-paths for nsINode, ...), but the numbers don't move. Compare https://tbpl.mozilla.org/?tree=Try&rev=9b8332283266 (pre bug 812333) with https://hg.mozilla.org/try/rev/8b8050302e54 (post bug 812333) and https://tbpl.mozilla.org/?tree=Try&rev=f3ef1dde412b (post bug 812333 with improvements).
I don't have a Linux or Windows profiling setup yet.
Assignee | ||
Comment 6•12 years ago
|
||
Though the result is confusing, almost all the numbers in the dromaeo-dom for https://tbpl.mozilla.org/php/getParsedLog.php?id=17313052&tree=Try&full=1 look higher than those in https://tbpl.mozilla.org/php/getParsedLog.php?id=17299409&tree=Try&full=1 which suggests the patches are helping, but the overall number on tbpl is lower.
Assignee | ||
Comment 7•12 years ago
|
||
Hmmm, looking at the raw logs it looks like the patches do help:
Linux:
before bug 812333:
dromaeo_css: 2172.02
dromaeo_dom: 149.48
after bug 812333:
dromaeo_css: 2149.52
dromaeo_dom: 144.1
after bug 812333 with patches:
dromaeo_css: 2645.53
dromaeo_dom: 196.25
Windows:
before bug 812333:
dromaeo_css: 2643.89
dromaeo_dom: 191.5
after bug 812333:
dromaeo_css: 2633.18
dromaeo_dom: 190.27
after bug 812333 with patches:
dromaeo_css: 2645.53
dromaeo_dom: 196.25
No idea why tbpl is showing me different numbers :-/.
I guess we should get those patches in.
Assignee | ||
Comment 8•12 years ago
|
||
qsObjectHelper doesn't QI if you give it the nsWrapperCache, uses ToSupports and ToCanonicalSupports to avoid QIing and refcounting, and it can get the classinfo and scriptable helper quickly from nsINode. We could add ToSupports/ToCanonicalSupports for nsXHREventTarget and nsXMLHttpRequestUpload, but I think they've been in the tree long enough to unpref them and avoid fallback to XPConnect wrapping.
Assignee | ||
Comment 9•12 years ago
|
||
This sucks a bit but IsDOMBinding() should be faster than calling the virtual WrapObject(...), so while we have our hybrid setup (new bindings and XPConnect wrappers) this is a slight win. I guess I should file a followup to undo this.
Attachment #684855 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•12 years ago
|
||
castNativeFromWrapper had gotten slightly slower from bug 807330, this improves it a little bit.
Attachment #684856 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #684857 -
Flags: review?(bzbarsky)
Comment 12•12 years ago
|
||
Comment on attachment 684855 [details] [diff] [review]
Avoid calling WrapObject v1
Review of attachment 684855 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bindings/BindingUtils.h
@@ +500,5 @@
> +// Create a JSObject wrapping "value", if there isn't one already, and store it
> +// in *vp. "value" must be a concrete class that implements a GetWrapper()
> +// which can return its existing wrapper, if any, and a WrapObject() which will
> +// try to create a wrapper. Typically, this is done by having "value" inherit
> +// from nsWrapperCache.
"value" must also implement IsDOMBinding now.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #7)
> Linux:
>
> before bug 812333:
>
> dromaeo_css: 2172.02
> dromaeo_dom: 149.48
>
> after bug 812333:
>
> dromaeo_css: 2149.52
> dromaeo_dom: 144.1
>
> after bug 812333 with patches:
>
> dromaeo_css: 2645.53
> dromaeo_dom: 196.25
That last one was wrong, it's
dromaeo_css: 2140.77
dromaeo_dom: 146.46
which actually doesn't look much better :-/.
Reporter | ||
Comment 14•12 years ago
|
||
OK, so the "linux64" builds I tried before were actually 32-bit Linux builds. And in 32-bit Linux builds what I was seeing was that JS_THIS didn't get inlined and gcc generated an 8-byte memmove for the copy constructor of JS::Value (!).
Using JS_ALWAYS_INLINE on JS_THIS helped with that.
On an actual 64-bit Linux build, I can't obviously seem to reproduce the problem locally.
Reporter | ||
Comment 15•12 years ago
|
||
Comment on attachment 684854 [details] [diff] [review]
Use qsObjectHelper v1
The Bindings.conf changes seem to have strayed in, but they're OK. r=me
Attachment #684854 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 16•12 years ago
|
||
Comment on attachment 684855 [details] [diff] [review]
Avoid calling WrapObject v1
The comments on WrapNewBindingObject should talk about GetWrapperPreserveColor (my fault there) and should mention the need for IsDOMBinding().
r=me with that
Attachment #684855 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 17•12 years ago
|
||
Comment on attachment 684856 [details] [diff] [review]
Reorder castNativeFromWrapper v1
r=me
Attachment #684856 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 18•12 years ago
|
||
Comment on attachment 684857 [details] [diff] [review]
Inline UnwrapObject and xpc_qsUnwrapObj v1
r=me
Attachment #684857 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 19•12 years ago
|
||
I filed bug 815460 on the JS_THIS thing.
Assignee | ||
Comment 20•12 years ago
|
||
Comment 21•12 years ago
|
||
Backed out because of test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/5de5d3950328
Assignee | ||
Comment 22•12 years ago
|
||
Parent wrapping was also calling WrapObject, so needed to check CouldBeDOMBinding.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db4085a10b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2997ef8891c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae93793daecd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d678d658159
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3db4085a10b2
https://hg.mozilla.org/mozilla-central/rev/2997ef8891c4
https://hg.mozilla.org/mozilla-central/rev/ae93793daecd
https://hg.mozilla.org/mozilla-central/rev/7d678d658159
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
•