190746 - Add a "forget/release" method to nsCOMPtr

Status: RESOLVED DUPLICATE of bug 78016

(Core :: XPCOM, enhancement)

Description

[David Bradley]

There are numerous, probably thousands, places where code is forced to do an
NS_ADDREF becaused there's no way to have nsCOMPtr give up it's reference. Those
creates a needless addref/release pair.

In testing, such a function would remove more than 17,000 addref/release pairs
from startup and shutdown. And around 13,000 for a page like www.cnn.com.

Comment 1

[David Bradley]

Attachment: Addes forget, and replaces NS_ADDREF pattern

This patch is large, and probably touches on things that are old, or things
that shouldn't be touched, but it's a starting point. Unfortunately I'm sure it
will go stale pretty quick.

Also, I believe it introduces a crash. I'm not sure if the crash is truly due
to the changes here, or just uncovered.

There are a few places where I took the liberty of cleaning some things up, so
I may go back and take out some of the changes that are more than just adding
forget.

The other issue is that more than a patch will be needed. People will have to
become familiar with the pattern and start using it, else we'll just get more
of the old pattern.

Comment 2

[Scott Collins]

Are you saying you want the |nsCOMPtr| to forget to |Release| the thing it's
pointing to, so that you can return an |AddRef|ed result?  If so, what you are
really complaining about is that the normal form of getter returns is
inconvenient, right?  We are hunting for a solution to this problem in bug
#178182 though much of the discussion can be found in

  http://bugzilla.mozilla.org/show_bug.cgi?id=172030#c18

The _one_ thing |nsCOMPtr| does is manage ownership of a single resource.  To my
eye, it seems like adding a |forget| member function subverts the single purpose
of |nsCOMPtr|.  Are you in love with |forget|?  Or would you be happy with any
solution that solved the `|AddRef| on exit' problem?

Comment 3

[Scott Collins]

On the other hand, this solution may be simpler than any suggested yet in the
aforementioned bugs.  It certainly bears thinking about.  I'll make this bug
block the |nsCOMPtr| tracking bug #178174 and I'll link to this bug from the
`|AddRef| on return' bug #178182

Comment 4

[David Bradley]

No, I'm more than open to alternative solutions. In my view, the goal is to
remove the unecessary addref/release that occurs with the current pattern. If
there are better alternatives that achieve that, then I'd be more than happy
with that. Adding "forget" was fairly easy and allowed me to see how big the
problem was and if it was even worth considering. dbaron has added forget to
nsRefPtr. I think we should try and keep the usage of the two fairly similar if
possible. And I don't think that's an argument to force forget into nsCOMPtr,
but just noting that nsRefPtr should probably follow what ever is done to
nsCOMPtr. I'll take a look a the other bugs to see what the alternatives are.

Comment 5

[John Keiser (jkeiser)]

This is actually bug 178187.

Returning from a function is not the only place where this is useful.  There is
at least one spot where I converted raw pointers over to nsCOMPtr where I had to
do an unnecessary addref / release because of ownership transfer *within* a
function:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp#3823

notifyContent[4] = mDragOverContent;
NS_IF_ADDREF(notifyContent[4]); // addref
mDragOverContent = aContent; // release, addref

With forget() this could turn into:
notifyContent[4] = mDragOverContent.forget();
mDragOverContent = aContent; // addref

As we hashed out over a long period of time, a function like
accept_ownership_of() is probably a best bet.

Comment 6

[John Keiser (jkeiser)]

Hrm, perhaps it's not precisely the same as that other bug, but it should be--if
you transfer ownership out of the COMPtr you should not release the pointer, so
nulling it out is a very viable option.

And I love the fact that this patch fixes so many callers.  Mega-kudos.

Comment 7

[David Bradley]

My main goal was to try and find out how much a perf benefit removing the
unnecessary addref/release pair from the pattern. 13k for a page seems
substantial, but to get that requires a lot of modifications to code and the
associated risk. Also it would be nice something besides forget is the ideal
solution that, dbaron's nsRefPtr gets that addition as well before it gets
turned on and used.

P.S. I've had the flu for the past week, and need something mindless to do in
the evenings ;-)

Comment 8

[John Keiser (jkeiser)]

Welll, your patch is just about minimum risk.  accept_ownership_of() or
something similar would be too.  The risk of changing the callers to be more
efficient is just something we'll be forced to take.

Comment 9

[Scott Collins]

John, your snippet from |nsEventStateManager| is not a good argument, because
|notifyContent| innappropriately manages references to COM objects with raw
pointers, and requires hand |Release|ing at the end of the function.  If
|notifyContent| were an array of |nsCOMPtr|s, then we could have used the |swap|
function proposed in bug #78016

I recognize the humor in your comment beginning with "Welll, your patch is just
about minimum risk.", but someone just reading through these comments might
believe you meant that at face value.  Obviously a patch that induces a crash is
nowhere near minimal risk.  And we'd probably want to split this patch into at
least two pieces anyway, one to add the functionality to |nsCOMPtr|, and another
to start exploiting it.

I like |forget|, but I like |swap| better between to |nsCOMPtr|s.  In a way,
|forget| is a one-way version of |swap|.  I actually think I |swap| might be
reasonable between a raw pointer and an |nsCOMPtr| as well.  In which case,
|swap|ing with a raw pointer variable that initially has the value |0| is
equivalent to |forget|.  Consider:

  nsIFoo* result = 0;
  nsCOMPtr<nsIFoo> foo = ...
  // ... lot's of work, possibly altering |foo|
  foo.swap(&result);
  return result;

Along the |swap| road, many additional signatures occur to me, including forms
that would be syntactically identical to |forget|... but I don't know that we
really need any of them given

  void nsCOMPtr<T>::swap( nsCOMPtr<T>& );
  void nsCOMPtr<T>::swap( T** );
    // or perhaps, for consistency
  void nsCOMPtr<T>::swap( T*& );

Comment 10

[Scott Collins]

Please excuse the bad spelling and typos in the post above... I've got the flu
and the medication is making me stupid.  Thinking about |swap|, though, has me
wondering if this isn't the solution to several of the annoyances about
|nsCOMPtr| we've been wanting to fix.

Comment 11

[Scott Collins]

Although, you know

  T* nsCOMPtr<T>::swap( T* );

has a certain charm to it.  The downside is the potential for leaks.  In the
form given a couple of comments back, the old managed pointer is stuffed into
the in/out parameter, and so we have at least some guarantee that the caller has
not lost the pointer.  In this form, though it's pretty and let's us say, e.g.,

  return foo.swap(0); // very convenient, but not very communicative

or

  result = foo.swap(result); // less convenient, more communicative
  return result;

You can see how easy it would be to drop the result.

  foo.swap(0); // leak

...it's convenient, yes, but I think much too sloppy.

Comment 12

[Scott Collins]

And since with the sloppy signature just above,

  foo.swap(0); // is identical in meaning to
  foo.forget();

you can see that |forget()| is the way to get |nsCOMPtr|s to `leak on command'.

Comment 13

[John Keiser (jkeiser)]

scc is right, swap() would be a better alternative for my case, if only because
the raw pointer should be null, and therefore null will be transferred into the
COMPtr.  So is returning a COMPtr really the only case we're trying to deal with
here?

Comment 14

[David Bradley]

swap sounds much more useful than forget, I like it.

Comment 15

[Scott Collins]

After discussion on irc with dbradley, marking this bug a duplicate of bug
#78016 and upping that bug's priority.  We need |swap|.

*** This bug has been marked as a duplicate of 78016 ***