213602 - Switch some users of NS_GetWeakReference over to do_GetWeakReference

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[jag (Peter Annema)]

Comment 1

[jag (Peter Annema)]

Attachment: switch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 128364 [details] [diff] [review]
switch

sr=dbaron.  Hopefully the performance hit will be insignificant.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128364 [details] [diff] [review]
switch

r=bzbarsky

Comment 4

[jag (Peter Annema)]

I guess I should look at the performance aspect a little more, though it seems
like both construct a nsGetWeakReference object and use its operator() to do the
work.

Comment 5

[jag (Peter Annema)]

// non-inlined:
nsIWeakReference*
NS_GetWeakReference( nsISupports* aInstancePtr, nsresult* aErrorPtr = 0 )
  {
    void* result = 0;
    nsGetWeakReference(aInstancePtr, aErrorPtr)(NS_GET_IID(nsIWeakReference),
&result);
    return NS_STATIC_CAST(nsIWeakReference*, result);
  }

vs.

// inlined:
const nsGetWeakReference                                                       
                    
do_GetWeakReference( nsISupports* aRawPtr, nsresult* error = 0 )
  {                                                                            
                    
    return nsGetWeakReference(aRawPtr, error);                                 
                    
  } 

NS_GetWeakReference can by-pass a virtual function call because it calls
operator() on a concrete object vs. nsCOMPtr calling it on an interface. I'll
have to compile and see what effect the other variables (non-inline vs. inline,
return value vs. out param) have. If there actually is a non-negligible impact,
could we change do_GetWeakReference to be (the equivalent of)?

  already_AddRefed<nsIWeakReference>
  do_GetWeakReference( nsISupports* aRawPtr, nsresult* error = 0 )
    {
      return NS_GetWeakReference(aRawPtr, error);
    }

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, I didn't realize NS_GetWeakReference was already using nsGetWeakReference.

Comment 7

[jag (Peter Annema)]

This did increase the codesize a little 'coz of nsGetWeakReference constructor
getting inlined, and the nsCOMPtr constructor changing, adding a NS_GET_IID. 

Comment 8

[jag (Peter Annema)]

Well, not the nsCOMPtr constructor changing, but rather a change in which
nsCOMPtr constructor we call.

Comment 9

[jag (Peter Annema)]

Attachment: No need for nsGetWeakReference as a nsCOMPtr_helper

Alternatively I could reverse this and implement NS_GetWeakReference in terms
of do_GetWeakReference (does already_AddRefed<...> play nice with NS_COM?).

Comment 10

[jag (Peter Annema)]

Attachment: I had to make this change

Changing do_GetWeakReference to return already_AddRefed<nsIWeakReference>
nsPrefBranch.cpp to break. The old code was doing an implicit upcast from
nsIWeakReference to nsISupports.

Comment 11

[jag (Peter Annema)]

Comment on attachment 128971 [details] [diff] [review]
I had to make this change

Btw, in a debug build the previous patch turned out to increase codesize, not
decrease it. Looking at optimized now.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 128971 [details] [diff] [review]
I had to make this change

Why did you need to change all this?  There's no need for the canonical
nsISupports pointer here, is there?

Comment 13

[jag (Peter Annema)]

I think it's needed when the observer is removed in ::RemoveObserver:

       if (pCallback->pObserver == observerRef) {

but correct me if I'm wrong.

Comment 14

[jag (Peter Annema)]

Comment on attachment 128971 [details] [diff] [review]
I had to make this change

Never mind, this would cause us to assert a lot in debug builds. Perhaps we
could just get rid of the assert and make this more like QueryInterface (i.e.
QI'ing to a non-supported interface isn't considered a bad thing), where a null
result tells us whether the do_GetWeakReference succeeded.

Comment 15

[jag (Peter Annema)]

Attachment: Smaller fix, more like what the other places do

I'd like to go with this patch for now. dbaron, jst and I have discussed
several solutions for this that would let us avoid duplicating the QI to
nsISupportsWeakReference (in the Add/RemoveObserver code and in the
*_GetWeakReference code), but I don't think we've settled on one solution
(unless we settled on two yesterday).

Comment 16

[jag (Peter Annema)]

Oh, so here are the thoughts (should probably go to a new bug, really):

1) Just remove the assertion in NS_GetWeakReference, trying to get a weak
reference from something that doesn't support it is okay, we'll return nsnull
and the appropriate error value to indicate it's not supported (more like how
QueryInterface works).

2) Add a second version of NS_GetWeakReference and do_GetWeakReference that take
a nsISupportsWeakReference* so the client itself can QI to find out if the
object supports weak references and avoid doing an (in this case) redundant QI.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 129305 [details] [diff] [review]
Smaller fix, more like what the other places do

-    observerRef = do_GetWeakReference(weakRefFactory);
+    observerRef = dont_AddRef(NS_STATIC_CAST(nsISupports*,
NS_GetWeakReference(weakRefFactory)));

NS_GetWeakReference() returns an nsIWeakReference, which inherits directly from
nsISupports. So why bother casting, the compiler will do that for you.

-	  observerRef = do_GetWeakReference(aObserver);
+	  observerRef = dont_AddRef(NS_STATIC_CAST(nsISupports*,
NS_GetWeakReference(aObserver)));

Same here.

r=jst either way.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The cast is needed because nsCOMPtr<T>& operator=( const already_AddRefed<T>&
rhs ) requires an already_AddRefed<T> of the same type.

Comment 19

[jag (Peter Annema)]

Comment on attachment 128964 [details] [diff] [review]
No need for nsGetWeakReference as a nsCOMPtr_helper

Requesting a=

Small change, should make us a little smaller here and there.

Comment 20

[jag (Peter Annema)]

Comment on attachment 129305 [details] [diff] [review]
Smaller fix, more like what the other places do

Requesting a=

Need this to make it compile.

Comment 21

[Asa Dotzler [:asa]]

Do we need both of these patches? When this gets landed let's make sure we watch
for any performance inpact.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 128964 [details] [diff] [review]
No need for nsGetWeakReference as a nsCOMPtr_helper

->1.6a

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 129305 [details] [diff] [review]
Smaller fix, more like what the other places do

->1.6a