1393592 - deCOM nsIWeakReference

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

We've seen (bug 1390527, bug 1346723) that nsWeakReference::QueryReferent can show up in profiles of Speedometer, being a virtual call.  Given that nsIWeakReference isn't *really* a script-accessible thing, there's no need to go through an IDL interface for this sort of thing.  We can do it all in virtual call-free C++, by collapsing nsIWeakReference and nsWeakReference into a single class.

The assumption here is that doing that conversion is easier than trying to convert some subset of call sites to use MFBT's weak references, since nsIWeakReference and nsWeakReference by definition share the same set of interfaces.  Converting should be a matter of twiddling with nsWeakReference to devirtualize things, export it via some appropriate header, and then substituting the new name at various places.  We'd obviously like to have a single, standard pattern for weak references throughout the tree, but one step at a time!

Hard bits that I haven't thought through yet:

1) nsIWeakReference has *two* implementing subclasses: nsWeakReference and nsNodeWeakReference...but it looks like they both implement the nsIWeakReference interface in basically identical ways, so maybe it will be straightforward to merge the two.

2) nsIWeakReference is tied to the nsISupportsWeakReference interface, which is a sprawling monstrosity (references in JS/XUL/etc.) and also tied into how xpconnect does weak references.  So deCOMing nsIWeakReference by the path sketched out above may requiring venturing into some very unfriendly waters.

3) Probably other things.

Given #2 above, maybe the easier approach is something like:

1) Give nsIWeakReference a pointer member, basically lifting nsWeakReference::mReferent and nsNodeWeakReference::mNode into nsIWeakReference.

2) Alter the nsIWeakReference interface to have something like:

  [binaryname(QueryReferentFromScript)]
  void QueryReferent( in nsIIDRef uuid, [iid_is(uuid), retval] out nsQIResult result );

  // Or whatever the correct incantation for a non-virtual method declared from
  // IDL is these days.
  [noscript, noxpcom, binaryname(QueryReferent)]
  void QueryReferentFromC( in nsIIDRef uuid, [iid_is(uuid), retval] out nsQIResult result );

3) QueryReferentFromC (called as QueryReferent from C++) then becomes the primary implementation method, and QueryReferent (called as QueryReferentFromScript from C++) will just call the *C++* QueryReferent.  Then everything that uses weak references will magically use non-virtual calls.

4) Adjust nsWeakReference and nsNodeWeakReference to deal with the pointer they were managing now living in a superclass.

I know that doesn't fulfill Ehsan's original goal of eliminating both the QueryReferent virtual call and the QI *inside* QueryReferent--this scheme would not eliminate the QI--but given that all this ties into xpconnect, I'm not sure we can do much better here--at least not in a timescale that would let us ship in 57.  WDYT?

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: manually de-virtualize nsIWeakReference::QueryReferent

The (second!) proposal from comment 0 is actually a little easier to express in
code, and doesn't take very long to write, so I've done that.  WDYT?

Comment 2

[(no longer active)]

I think I like these ideas, thanks a lot for thinking through them!

Comment 3

[(no longer active)]

Comment on attachment 8900917 [details] [diff] [review]
manually de-virtualize nsIWeakReference::QueryReferent

Review of attachment 8900917 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice.

::: xpcom/base/nsIWeakReference.idl
@@ +38,4 @@
>      void QueryReferent( in nsIIDRef uuid, [iid_is(uuid), retval] out nsQIResult result );
>  
>  %{C++
> +  virtual size_t SizeOfOnlyThis(mozilla::MallocSizeOf aMallocSizeOf) const = 0;

Not quite sure what changed on this line?

@@ +47,5 @@
> +  {
> +    return !!mObject;
> +  }
> +
> +  nsresult QueryReferent(const nsIID& aIID, void** aInstancePtr);

Would it be possible to inline this?  (Could be a follow-up...)

::: xpcom/base/nsWeakReference.cpp
@@ +149,5 @@
>  
> +  if (!mObject) {
> +    return NS_ERROR_NULL_POINTER;
> +  }
> +  

Nit: trailing whitespace!

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: manually de-virtualize nsIWeakReference::QueryReferent

Had to move the threadsafety assertions around.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3)
> ::: xpcom/base/nsIWeakReference.idl
> @@ +38,4 @@
> >      void QueryReferent( in nsIIDRef uuid, [iid_is(uuid), retval] out nsQIResult result );
> >  
> >  %{C++
> > +  virtual size_t SizeOfOnlyThis(mozilla::MallocSizeOf aMallocSizeOf) const = 0;
> 
> Not quite sure what changed on this line?

Spacing, I think.

> @@ +47,5 @@
> > +  {
> > +    return !!mObject;
> > +  }
> > +
> > +  nsresult QueryReferent(const nsIID& aIID, void** aInstancePtr);
> 
> Would it be possible to inline this?  (Could be a follow-up...)

This gets inlined due to unified sources, but I guess we could make it defined in the header...

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7137e75e62
manually de-virtualize nsIWeakReference::QueryReferent; r=ehsan

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c7137e75e62