285404 - faster access to XPConnectWrappedNative's native pointer

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Brian Ryner (not reading)]

To optimize things a bit for native callers (are there non-native callers?  the
interface isn't scriptable...) we can add an inline accessor for
nsIXPConnectWrappedNative's nsISupports pointer.

I also added some nsCOMPtr machinery to facilitate automatic QI of the native
object to a given interface.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Brian Ryner (not reading)]

Attachment: -w patch for reviewing

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 176844 [details] [diff] [review]
patch

Actually, I think the do_QueryWrappedNative can be done more efficiently using
nsQueryInterface.  Investigating now...

Comment 4

[Brian Ryner (not reading)]

Attachment: patch

Comment 5

[Brian Ryner (not reading)]

Attachment: -w patch for reviewing

This patch reuses nsQueryInterface.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 176845 [details] [diff] [review]
-w patch for reviewing

removing obsolete request

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 176853 [details] [diff] [review]
-w patch for reviewing

- In js/src/xpconnect/src/xpccomponents.cpp:

Looks like you're leaving unused "sup" variables around.

- In cryptojs_GetObjectPrincipal:

-	   xpcNative->GetNative(getter_AddRefs(supports));
-	   objPrin = do_QueryInterface(supports);
+	   objPrin = do_QueryWrappedNative(xpcNative);

This was the only reason "supports" had to be a nsCOMPtr, you could now make it
a raw nsISupports pointer and save on some refcounting.

r=jst with that fixed.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 176853 [details] [diff] [review]
-w patch for reviewing

dbaron's out on vacation for the moment, darin do you think you could sr?

Also, after seeing jst's comment about removing unused variables in
xpccomponents.cpp, I found and removed several unused "native" variables in
nsDOMClassInfo.cpp.

Comment 9

[Darin Fisher]

Comment on attachment 176853 [details] [diff] [review]
-w patch for reviewing

sr=darin

maybe comment in the IDL file that Native() never returns null.

Comment 10

[David Bradley]

Looks good, what's the numbers on the speed up?

Comment 11

[Brian Ryner (not reading)]

Attachment: follow-up to make things even faster

Per conversation with jst, since we know nsWindowSH will only be used for
wrapped GlobalWindow objects, and likewise for nsDocumentSH and
nsHTMLDocumentSH, we can avoid QI altogether and just cast through the
appropriate interface.	I added a DEBUG check to warn if anything really bad
happens.

We could investigate doing this for some of the other SH classes as well, but
I'd need to verify that all of the relevant element impls return the same
interface when QI'd to nsISupports.

Comment 12

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050310
Firefox/1.0+

this build is crashing like mad.
talkback TB4243416X  points at XPCWrappedNativeTearOff

Comment 13

[Peter van der Woude [:Peter6]]

repro:
1. download latest hourly build FF
2. install
3. open FF
4. open multiple tabs including one to login at Gmail
5. login and crash (100% of the time)
no extensions, default theme.
also TB4244313Q (which is the same)

Comment 14

[Pavel Penaz]

This bug probably caused the crash in bug 285579

Comment 15

[David Bradley]

Again, do we have any numbers for this perf improvement? Does it really speed
anything up by any measurable amount?

Comment 16

[Worcester12345]

Someone please add perf key word.

Comment 17

[David Bradley]

I thought I did with my last comment, trying again.

Comment 18

[Peter van der Woude [:Peter6]]

This bug probably caused the crash in bug 285546

Comment 19

[Dan M]

And I'm going to try to blame it for bug 285691, though I haven't verified that.
However the patch for bug 285546 fixes bug 285691.

Comment 20

[David Bradley]

The datamember on the interface seems a bit odd. Are we deciding to break that
rule in favor of performance now?

Took a look at tinderbox and there isn't any dicernable improvement from this
patch other than code size.

Maybe we need a non-com based holder that delivers the performance we need? I
know that's not an answer for holders passing through interface methods. I don't
know what the break out is on what holders pass through interface methods and
those that are used locally.

You could get a similar code reduction by using the same specialized QI but with
a virtual method instead of an inline method. And I think this would still
eliminate the add/ref pair as does the original patch. I wonder if the virtual
call is even that big of a player in this scenario.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 176978 [details] [diff] [review]
follow-up to make things even faster

r+sr=jst

Comment 22

[Brian Ryner (not reading)]

Let me try to explain the rationale for this, in hopes of avoiding more peanuts
being thrown from the gallery.  I never claimed that this patch by itself would
help anything performance-wise right now.  However, based on the design I'm
implementing for bug 274784, this would likely show up as a hotspot as we unwrap
the native to retrieve the inner property store on any get or set of a property.
 I decided to optimize this pathway first in order to minimize the effect on
load/DHTML performance when 274784 lands.

Comment 23

[David Bradley]

Thanks for the explanation, the way I read the summary it implicity made that
claim and never mentioned that bug. I'll generally resist adding complexity
without having a decent chance of gaining something. Let me know if there's
anything I can do to help.

Comment 24

[Worcester12345]

(In reply to comment #22)
> Let me try to explain the rationale for this, in hopes of avoiding more peanuts
> being thrown from the gallery.  I never claimed that this patch by itself would
> help anything performance-wise right now.  However, based on the design I'm
> implementing for bug 274784, this would likely show up as a hotspot as we unwrap
> the native to retrieve the inner property store on any get or set of a property.
>  I decided to optimize this pathway first in order to minimize the effect on
> load/DHTML performance when 274784 lands.
> 
Should 274784 then depend on this bug?

Comment 25

[Brian Ryner (not reading)]

This is no longer a dependency for the back/forward cache.  We decided to simply
copy the properties.

Comment 26

[Worcester12345]

(In reply to comment #25)
> This is no longer a dependency for the back/forward cache.  We decided to simply
> copy the properties.

INVALID?

Comment 27

[Peter Van der Beken [:peterv]]

Should we make do_QueryWrappedNative work with a null pointer? Right now it
doesn't (it calls aWrappedNative->Native()), which makes it different from the
other do_Query*'s.

Comment 28

[Brian Ryner (not reading)]

closing, it's fast enough.