Closed Bug 814821 Opened 7 years ago Closed 7 years ago

Dromaeo dom-traverse regression from bug 812333

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: peterv)

References

Details

Attachments

(6 files)

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.
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.
Attached file Microbenchmark
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?
Blocks: 812333
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.
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.
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.
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: nobody → peterv
Status: NEW → ASSIGNED
Attachment #684854 - Flags: review?(bzbarsky)
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)
castNativeFromWrapper had gotten slightly slower from bug 807330, this improves it a little bit.
Attachment #684856 - Flags: review?(bzbarsky)
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.
(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 :-/.
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.
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+
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+
Comment on attachment 684856 [details] [diff] [review]
Reorder castNativeFromWrapper v1

r=me
Attachment #684856 - Flags: review?(bzbarsky) → review+
Comment on attachment 684857 [details] [diff] [review]
Inline UnwrapObject and xpc_qsUnwrapObj v1

r=me
Attachment #684857 - Flags: review?(bzbarsky) → review+
Depends on: 815460
I filed bug 815460 on the JS_THIS thing.
Depends on: 818281
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.