Why are PassRefPtr<>s used as function parameters?

[Jens Alfke-4]

Looking at how refcounting is implemented in WebCore, I was surprised  
to find that there are a lot of functions/methods that take  
PassRefPtr<>s as parameters instead of regular pointers to those  
objects. I can't see any benefit to this, and it adds the overhead of  
a ref() and deref() at every call-site.

For example in HTMLNameCollection.h:
   HTMLNameCollection(PassRefPtr<Document>, CollectionType, const  
String& name);

Why shouldn't this be
   HTMLNameCollection(Document*, CollectionType, const String& name);
?

The use of PassRefPtr instead of RefPtr here also seems prone to  
trouble, since inside the implementation of the method it could end up  
unexpectedly clearing the parameter:
   HTMLNameCollection::HTMLNameCollection(Document* doc, .... {
        Ref<Document> otherDoc = doc; // This sets doc to NULL!
        doc->something(); // CRASH
}

I ran across this while trying to track down a reference leak of a  
Document object. As one of my last resorts I set a watchpoint on the  
object's m_refcount to see who refs/derefs it; but I had to give up  
because so many method calls, including the one above, keep constantly  
twiddling the refcount while passing the Document as a parameter.

—Jens
_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Darin Adler]

On Oct 27, 2009, at 10:55 AM, Jens Alfke wrote:

> Looking at how refcounting is implemented in WebCore, I was  
> surprised to find that there are a lot of functions/methods that  
> take PassRefPtr<>s as parameters instead of regular pointers to  
> those objects. I can't see any benefit to this, and it adds the  
> overhead of a ref() and deref() at every call-site.

Have you read the RefPtr document? It is at <http://webkit.org/coding/RefPtr.html 
 >.

Only functions that take ownership of the passed-in objects should  
take PassRefPtr. This makes it so we can optimally handle the case  
where that object was just created and lets us hand off the reference  
instead of having to do a round of reference count thrash. If the  
function is going to be storing the value in a RefPtr, then the  
PassRefPtr does not introduce any reference count thrash even if the  
passed in object was not just created.

It’s wrong for the HTMLNameCollection constructor to take a PassRefPtr  
for its document argument because it doesn’t take ownership of the  
document. So that one is definitely a mistake.

It is true that PassRefPtr arguments can be tricky as you mentioned  
and the RefPtr document also mentions.

     -- Darin

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Drew Wilson-3]

In this case, HTMLCollection() keeps a reference to the object, so you can't safely pass in just a raw pointer. Since HTMLCollection keeps it around, you'd have to do the ref anyway.

-atw

On Tue, Oct 27, 2009 at 10:55 AM, Jens Alfke <snej@...> wrote:

Looking at how refcounting is implemented in WebCore, I was surprised to find that there are a lot of functions/methods that take PassRefPtr<>s as parameters instead of regular pointers to those objects. I can't see any benefit to this, and it adds the overhead of a ref() and deref() at every call-site.

For example in HTMLNameCollection.h:
 HTMLNameCollection(PassRefPtr<Document>, CollectionType, const String& name);

Why shouldn't this be
 HTMLNameCollection(Document*, CollectionType, const String& name);
?

The use of PassRefPtr instead of RefPtr here also seems prone to trouble, since inside the implementation of the method it could end up unexpectedly clearing the parameter:
 HTMLNameCollection::HTMLNameCollection(Document* doc, .... {
       Ref<Document> otherDoc = doc; // This sets doc to NULL!
       doc->something(); // CRASH
}

I ran across this while trying to track down a reference leak of a Document object. As one of my last resorts I set a watchpoint on the object's m_refcount to see who refs/derefs it; but I had to give up because so many method calls, including the one above, keep constantly twiddling the refcount while passing the Document as a parameter.

—Jens
_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Darin Adler]

On Oct 29, 2009, at 12:57 PM, Drew Wilson wrote:

> HTMLCollection() keeps a reference to the object, so you can't  
> safely pass in just a raw pointer.

Good point, but strictly speaking that’s not true.

It’s always safe to pass a raw pointer. The PassRefPtr type for the  
argument is solely about optimization, not safety or correctness.

I see what you mean, though. HTMLCollection’s constructor does take a  
PassRefPtr so it would be OK to have HTMLNamedCollection be consistent  
with this. Although there’s no realistic chance the document will be a  
just-created object, so it’s not a great practical optimization.

> Since HTMLCollection keeps it around, you'd have to do the ref anyway.

Unfortunately, the get() inside the HTMLNameCollection’s constructor  
that is needed so we can call nameCollectionInfo with the same  
document pointer means that we do an extra round of reference count  
churn. And there’s no obvious way to rewrite it to eliminate that. So  
the PassRefPtr type on HTMLNameCollection’s constructor’s document  
argument currently does no good and a bit of harm.

     -- Darin

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Maciej Stachowiak]

On Oct 29, 2009, at 12:56 PM, Darin Adler wrote:

> On Oct 27, 2009, at 10:55 AM, Jens Alfke wrote:
>
>> Looking at how refcounting is implemented in WebCore, I was  
>> surprised to find that there are a lot of functions/methods that  
>> take PassRefPtr<>s as parameters instead of regular pointers to  
>> those objects. I can't see any benefit to this, and it adds the  
>> overhead of a ref() and deref() at every call-site.
>
> Have you read the RefPtr document? It is at <http://webkit.org/coding/RefPtr.html 
> >.
>
> Only functions that take ownership of the passed-in objects should  
> take PassRefPtr. This makes it so we can optimally handle the case  
> where that object was just created and lets us hand off the  
> reference instead of having to do a round of reference count thrash.  
> If the function is going to be storing the value in a RefPtr, then  
> the PassRefPtr does not introduce any reference count thrash even if  
> the passed in object was not just created.
>
> It’s wrong for the HTMLNameCollection constructor to take a  
> PassRefPtr for its document argument because it doesn’t take  
> ownership of the document. So that one is definitely a mistake.

However, its base class HTMLCollection can take ownership. The  
HTMLNameCollection constructor misses out on the optimization though:

HTMLNameCollection::HTMLNameCollection(PassRefPtr<Document> document,  
CollectionType type, const String& name)
     : HTMLCollection(document.get(), type, document-
 >nameCollectionInfo(type, name))
     , m_name(name)
{
}

Note that it passes document.get(), but also uses document after that  
so it couldn't just pass document. Thus, it probably shouldn't take a  
PassRefPtr (unless there is a clever way to fix it that I'm not seeing).

  - Maciej

>
> It is true that PassRefPtr arguments can be tricky as you mentioned  
> and the RefPtr document also mentions.
>
>    -- Darin
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@...
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Drew Wilson-3]

Aside from the ref-counter munging, it still seems like there's value in using PassRefPtr as a signifier that a given API takes ownership of a pointer.

So I'm not certain (in the case of HTMLNameCollection) that it'd be a better solution to change it not to take an ordinary pointer - yes, it would eliminate some ref churn, but at the cost of losing that connotation.

A better solution (since the constructor is private) would be to make the constructor explicitly take a CollectionCache* parameter, thereby allowing us to directly pass the document straight through to the base class.

-atw

On Thu, Oct 29, 2009 at 3:31 PM, Maciej Stachowiak <mjs@...> wrote:

On Oct 29, 2009, at 12:56 PM, Darin Adler wrote:

On Oct 27, 2009, at 10:55 AM, Jens Alfke wrote:

Looking at how refcounting is implemented in WebCore, I was surprised to find that there are a lot of functions/methods that take PassRefPtr<>s as parameters instead of regular pointers to those objects. I can't see any benefit to this, and it adds the overhead of a ref() and deref() at every call-site.

Have you read the RefPtr document? It is at <http://webkit.org/coding/RefPtr.html>.

Only functions that take ownership of the passed-in objects should take PassRefPtr. This makes it so we can optimally handle the case where that object was just created and lets us hand off the reference instead of having to do a round of reference count thrash. If the function is going to be storing the value in a RefPtr, then the PassRefPtr does not introduce any reference count thrash even if the passed in object was not just created.

It’s wrong for the HTMLNameCollection constructor to take a PassRefPtr for its document argument because it doesn’t take ownership of the document. So that one is definitely a mistake.

However, its base class HTMLCollection can take ownership. The HTMLNameCollection constructor misses out on the optimization though:

HTMLNameCollection::HTMLNameCollection(PassRefPtr<Document> document, CollectionType type, const String& name)
   : HTMLCollection(document.get(), type, document->nameCollectionInfo(type, name))
   , m_name(name)
{
}

Note that it passes document.get(), but also uses document after that so it couldn't just pass document. Thus, it probably shouldn't take a PassRefPtr (unless there is a clever way to fix it that I'm not seeing).

 - Maciej

It is true that PassRefPtr arguments can be tricky as you mentioned and the RefPtr document also mentions.

  -- Darin

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Jeremy Orlow-2]

My thinking is that we should be consistent.  And given that passing a raw pointer is equal to or faster than passing a PassRefPtr it seems to me that we should just do that whenever ownership isn't being transfered.  Note that this is what's done in the majority of cases I've seen and it's what the RefPtr doc recommends, IIRC.

I don't see why the distinction between a callee simply using an object or adopting it for future use is terribly interesting to the caller.

J

On Thu, Oct 29, 2009 at 3:55 PM, Drew Wilson <atwilson@...> wrote:

Aside from the ref-counter munging, it still seems like there's value in using PassRefPtr as a signifier that a given API takes ownership of a pointer.

So I'm not certain (in the case of HTMLNameCollection) that it'd be a better solution to change it not to take an ordinary pointer - yes, it would eliminate some ref churn, but at the cost of losing that connotation.

A better solution (since the constructor is private) would be to make the constructor explicitly take a CollectionCache* parameter, thereby allowing us to directly pass the document straight through to the base class.

-atw

On Thu, Oct 29, 2009 at 3:31 PM, Maciej Stachowiak <mjs@...> wrote:

On Oct 29, 2009, at 12:56 PM, Darin Adler wrote:

On Oct 27, 2009, at 10:55 AM, Jens Alfke wrote:

Looking at how refcounting is implemented in WebCore, I was surprised to find that there are a lot of functions/methods that take PassRefPtr<>s as parameters instead of regular pointers to those objects. I can't see any benefit to this, and it adds the overhead of a ref() and deref() at every call-site.

Have you read the RefPtr document? It is at <http://webkit.org/coding/RefPtr.html>.

Only functions that take ownership of the passed-in objects should take PassRefPtr. This makes it so we can optimally handle the case where that object was just created and lets us hand off the reference instead of having to do a round of reference count thrash. If the function is going to be storing the value in a RefPtr, then the PassRefPtr does not introduce any reference count thrash even if the passed in object was not just created.

It’s wrong for the HTMLNameCollection constructor to take a PassRefPtr for its document argument because it doesn’t take ownership of the document. So that one is definitely a mistake.

However, its base class HTMLCollection can take ownership. The HTMLNameCollection constructor misses out on the optimization though:

HTMLNameCollection::HTMLNameCollection(PassRefPtr<Document> document, CollectionType type, const String& name)
   : HTMLCollection(document.get(), type, document->nameCollectionInfo(type, name))
   , m_name(name)
{
}

Note that it passes document.get(), but also uses document after that so it couldn't just pass document. Thus, it probably shouldn't take a PassRefPtr (unless there is a clever way to fix it that I'm not seeing).

 - Maciej

It is true that PassRefPtr arguments can be tricky as you mentioned and the RefPtr document also mentions.

  -- Darin

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Geoffrey Garen]

> I don't see why the distinction between a callee simply using an  
> object or adopting it for future use is terribly interesting to the  
> caller.

Agreed. And let's avoid making it interesting!

The whole point of reference counting is to ensure that one object can  
reference a piece of data without knowing or caring whether another  
object references that data.

Geoff
_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev