Closed Bug 190746 Opened 22 years ago Closed 22 years ago

Add a "forget/release" method to nsCOMPtr

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 78016

People

(Reporter: dbradley, Assigned: dbradley)

References

Details

(Keywords: perf)

Attachments

(1 file)

Addes forget, and replaces NS_ADDREF pattern
363.07 KB, patch
Details | Diff | Splinter Review
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.
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.
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?
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
Blocks: nsCOMPtr_tracking
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.
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.
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.
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 ;-)
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.
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*& );
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.
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.
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'.
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?
swap sounds much more useful than forget, I like it.
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 ***
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: