Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 1

2 years ago
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?
Attachment #8900917 - Flags: review?(ehsan)

Comment 2

2 years ago
I think I like these ideas, thanks a lot for thinking through them!
Flags: needinfo?(ehsan)

Comment 3

2 years ago
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!
Attachment #8900917 - Flags: review?(ehsan) → review+
(Assignee)

Updated

2 years ago
Assignee: nobody → nfroyd
(Assignee)

Updated

2 years ago
Blocks: 1393768
(Assignee)

Comment 4

2 years ago
Had to move the threadsafety assertions around.
Attachment #8901197 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8900917 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
(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

2 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7137e75e62
manually de-virtualize nsIWeakReference::QueryReferent; r=ehsan

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c7137e75e62
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.