Should we allow ref counting const Foo? (Allows scoped_refptr<const Foo>)?

[William Chan (陈智昌)]

http://codereview.chromium.org/1575024/show has some of the discussion between me and Darin, and recently mpcomplete also chimed in.  We're interested in what chromium-dev has to say about this.

In certain situations, we'd like to reference count a const object.  Matt has a current example of this (http://codereview.chromium.org/3439017/show).  Typically, one would create the object, after which it would be immutable.  Afterwards, there may be a number of different consumers who use it.  There are a few options here.  First, you can just say you don't care about constness.  Second, without changing the refcounts to be mutable, you could write some extra code to produce a wrapper object with all const accessors and make this one refcounted.  The third option is to make the refcounts mutable so we can call AddRef()/Release() on RefCounted subtypes.

Some points Darin raised against making refcounts mutable:

* Inconsistent with COM and WebKit reference counting.

* Object deletion is basically object mutation.  If I pass const Foo* to a function, I don't really expect it to be deleted.

Some points in favor of making refcounts mutable:

* scoped_ptr<const Foo> is already allowed.  Why not scoped_refptr<const Foo>?

* It's debatable whether constness is related to lifetime.  constness says that an object doesn't change, but has nothing to do with lifetime.  And it's unlikely that the callee is going to end up deleting the const object (at least during the scope of the function call).

* It's annoying to have to write some boilerplate wrapper that only provides const accessors.  In general, less boilerplate is better.

Does anyone else have thoughts here?

[David Levin]

mutable refcounts make verifying multithreaded code much harder.

Using a (non-threadsafe) RefCounted object on multiple threads is an unfortunate situation, but it happens, and it would be nice if the coding structures helped one right code correctly.

dave

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Matt Perry]

On Tue, Sep 28, 2010 at 2:40 PM, David Levin <le...@chromium.org> wrote:

mutable refcounts make verifying multithreaded code much harder.

Can you explain how? To be clear, we already have thread-safe refcounting. We just can't use const on these objects. In my case, allowing it to be const makes it a lot easier to verify that we don't accidentally modify the shared object when we shouldn't.

[David Levin]

On Tue, Sep 28, 2010 at 2:50 PM, Matt Perry <mpcom...@chromium.org> wrote:

On Tue, Sep 28, 2010 at 2:40 PM, David Levin <le...@chromium.org> wrote:

mutable refcounts make verifying multithreaded code much harder.

Can you explain how? To be clear, we already have thread-safe refcounting. We just can't use const on these objects. In my case, allowing it to be const makes it a lot easier to verify that we don't accidentally modify the shared object when we shouldn't.

My specific issue is something I see in WebKit a bit.  Maybe it doesn't happen in chromium.

Details:

Strings in WebKit cannot be made to have thread-safe refcounting for various reasons. However, there have been several cases in which folks have put them in a data structure that is used from multiple threads. const ref counting would make it very easy to do non-threadsafe things.

Unfortunately, const hasn't helped in WebKit because strings contain a RefPtr to the StringImpl. The string being const doesn't matter to the StringImpl in them. Solution (which hasn't been done but should be): the code should use const StringImpl. non-mutable refcounting will ensure that the StringImpl being shared across threads doesn't get ref counted.

dave

 

[Peter Kasting]

I'm in favor of supporting scoped_refptr<const X>.

On Tue, Sep 28, 2010 at 1:44 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

Some points Darin raised against making refcounts mutable:

* Inconsistent with COM and WebKit reference counting.

COM isn't something most people on the team know.  WebKit is a bigger issue, but we already have differences; the question is the effect of differing here.  IMO, allowing const objects to be refcounted is a lot different than, say, "in one system the object is created with refs == 0, and in the other with refs == 1" where you need to be really careful to write different code in the two worlds.  Simply allowing a case WebKit doesn't seems fine to me because I don't see how it's going to cause lots of headaches.

 

* Object deletion is basically object mutation.  If I pass const Foo* to a function, I don't really expect it to be deleted.

I've never really agreed with this.  First, it's not _inherently_ wrong to delete a pointer-to-const-object at some point, because

const int* x = new int;

delete x;

...doesn't warn.

Second, to more directly address the example at hand, a function which deletes a pointer (const or not) must have ownership of the pointer, or something is going to go badly wrong.  If you are giving a function a pointer that it subsequently deletes, it should be only be because the expected contract is that you're giving that function ownership of the pointer.  When that's the case, I don't think you have a right to object to the function deleting the pointer (or doing anything else with it), unless the function is documented as preserving the pointer's life until some other time (in which case we don't run into the problem raised here).  This whole line of argument holds irrespective of anything's constness.

Some points in favor of making refcounts mutable:

* scoped_ptr<const Foo> is already allowed.  Why not scoped_refptr<const Foo>?

This has actually bitten me before and I was very confused as to why it wasn't consistent.

[Darin Fisher]

On Tue, Sep 28, 2010 at 2:40 PM, David Levin <le...@chromium.org> wrote:

mutable refcounts make verifying multithreaded code much harder.

Using a (non-threadsafe) RefCounted object on multiple threads is an unfortunate situation, but it happens, and it would be nice if the coding structures helped one right code correctly.

Assertions should fail if you try to pass a RefCounted object to NewRunnableMethod _and_ pass that Task across threads.

See RefCountedBase::ImplementsThreadSafeReferenceCounting.  Perhaps it would be good to add something like that to WTF::RefCounted as well.

-Darin

[Marc-Antoine Ruel]

The only issue I have with it is that many team members understand the different of const scoped_ptr<T> and scoped_ptr<const T> and their logical mapping to T* const and T const*. Ask someone with a Java background what it means and why they seem "inversed".

What I'm afraid of is the members that don't understand yet and yet fail to ask for help.

Maybe a wiki page with all Peter's knowledge dumped in directly linked in the scoped_refptr class definition?

M-A

[David Levin]

On Wed, Sep 29, 2010 at 8:55 AM, Darin Fisher <da...@chromium.org> wrote:
>
>
> On Tue, Sep 28, 2010 at 2:40 PM, David Levin <le...@chromium.org> wrote:
>>
>> mutable refcounts make verifying multithreaded code much harder.
>> Using a (non-threadsafe) RefCounted object on multiple threads is an
>> unfortunate situation, but it happens, and it would be nice if the coding
>> structures helped one right code correctly.
>
> Assertions should fail if you try to pass a RefCounted object to
> NewRunnableMethod _and_ pass that Task across threads.
> See RefCountedBase::ImplementsThreadSafeReferenceCounting.  Perhaps it would
> be good to add something like that to WTF::RefCounted as well.
> -Darin

I don't believe that this check would help in the cases that concern
me the most (where there is a data structure used in multiple threads)
like the database code for instance.

But it is a cool idea. I'll look at that more to see what equivalent
there could be for WebKit. Most of the cross thread task posting goes
through a centralized copy mechanism,
WebCore/platform/CrossThreadCopier.h. It doesn't have a specialization
for RefCounted, only the thread safe variant. However, it does allow
through pointers easily which in retrospec was a bad design decision
that I should fix.

Also, there has been work on something lower level for WebKit
(https://bugs.webkit.org/show_bug.cgi?id=31639) and I'm hopeful we'll
be able to put some effort there and land it in the not too distant
future.

dave

[William Chan (陈智昌)]

On Wed, Sep 29, 2010 at 9:24 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

The only issue I have with it is that many team members understand the different of const scoped_ptr<T> and scoped_ptr<const T> and their logical mapping to T* const and T const*. Ask someone with a Java background what it means and why they seem "inversed".

What I'm afraid of is the members that don't understand yet and yet fail to ask for help.

I think the same argument could be applied to const T* vs T const * vs T* const.  We don't ban those.  Are you concerned enough about this to completely outlaw scoped_refptr<const Foo>?  Or did you primarily want to point out this issue?

 

Maybe a wiki page with all Peter's knowledge dumped in directly linked in the scoped_refptr class definition?

More documentation is probably good.

[William Chan (陈智昌)]

Any other comments here?  It'd be nice to have stronger consensus if possible.  Currently, Matt, Peter, and I are definitely for the change, and Darin, Marc-Antoine, and David may or may not have reservations about the change, but are not against it per se?  Is this accurate?

[Marc-Antoine Ruel]

I'm fine with the change as I disagree that object deletion is object mutation.

Ref counting is a way to emulate a heap with GC. In no way in languages with a GC the refcounting is part of the object. For example 'const' objects like strings in python/java/C# are still "refcounted". In C++ we tend to optimize refcounting by keeping it inside the object to save one pointer and one heap allocation.

In C++ const object references tend to be a pain on the long run, especially when it starts to involve virtual functions and members acting as a cache. But that's exactly to fix race conditions caused by this kind of design that one would like to enforce const reference.

I'm also fine with the other pattern to use const members and use a build factory when necessary.

So in short I don't care either way.

M-A

Le 30 septembre 2010 20:06, Albert J. Wong (王重傑) <ajw...@chromium.org> a écrit :

I don't think there's anything inherently damaging about having mutable, but the high situations described where this seems interesting largely revolves around wanting to create immutable objects.

However, I also have any discussion about alternatives while preserving the status quo ref counting semantics.

How bad is it really to either force initialization via a constructor, or get people to apply a builder pattern (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets too complex?  That's how this is often dealt with in language that don't have const.

Having an API that fundamentally doesn't expose mutation is arguably an even stronger statement of "don't change me" than annotating a type with constness. You also get separation of initialization/validation logic from the "meat of the class."  The worst case cost is a builder object.

Note that I'm not suggesting creating a bunch of "read-only interfaces" that everything inherits from.

Is this style deemed too annoying?

-Albert

[Albert J. Wong (王重傑)]

I don't think there's anything inherently damaging about having mutable, but the high situations described where this seems interesting largely revolves around wanting to create immutable objects.

However, I also have any discussion about alternatives while preserving the status quo ref counting semantics.

How bad is it really to either force initialization via a constructor, or get people to apply a builder pattern (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets too complex?  That's how this is often dealt with in language that don't have const.

Having an API that fundamentally doesn't expose mutation is arguably an even stronger statement of "don't change me" than annotating a type with constness. You also get separation of initialization/validation logic from the "meat of the class."  The worst case cost is a builder object.

Note that I'm not suggesting creating a bunch of "read-only interfaces" that everything inherits from.

Is this style deemed too annoying?

-Albert

[Matt Perry]

On Thu, Sep 30, 2010 at 5:06 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

I don't think there's anything inherently damaging about having mutable, but the high situations described where this seems interesting largely revolves around wanting to create immutable objects.

However, I also have any discussion about alternatives while preserving the status quo ref counting semantics.

How bad is it really to either force initialization via a constructor, or get people to apply a builder pattern (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets too complex?  That's how this is often dealt with in language that don't have const.

Having an API that fundamentally doesn't expose mutation is arguably an even stronger statement of "don't change me" than annotating a type with constness. You also get separation of initialization/validation logic from the "meat of the class."  The worst case cost is a builder object.

Note that I'm not suggesting creating a bunch of "read-only interfaces" that everything inherits from.

Is this style deemed too annoying?

Yes. :) (Of course I speak only for myself.)

Also, using the builder pattern does not protect a class's methods from accidentally mutating its own parameters. Only const can do that. I will need to enforce this in a future change.

[William Chan (陈智昌)]

On Thu, Sep 30, 2010 at 5:10 PM, Marc-Antoine Ruel <mar...@google.com> wrote:

I'm fine with the change as I disagree that object deletion is object mutation.

Ok, thanks!

 

Ref counting is a way to emulate a heap with GC. In no way in languages with a GC the refcounting is part of the object. For example 'const' objects like strings in python/java/C# are still "refcounted". In C++ we tend to optimize refcounting by keeping it inside the object to save one pointer and one heap allocation.

Yeah, I think this is a case of an implementation detail leaking out into the interface.  shared_ptr doesn't have this restriction (and you don't have to have the separate heap allocation if you use make_shared).

 

In C++ const object references tend to be a pain on the long run, especially when it starts to involve virtual functions and members acting as a cache. But that's exactly to fix race conditions caused by this kind of design that one would like to enforce const reference.

I'm also fine with the other pattern to use const members and use a build factory when necessary.

So in short I don't care either way.

M-A

Le 30 septembre 2010 20:06, Albert J. Wong (王重傑) <ajw...@chromium.org> a écrit :

I don't think there's anything inherently damaging about having mutable, but the high situations described where this seems interesting largely revolves around wanting to create immutable objects.

However, I also have any discussion about alternatives while preserving the status quo ref counting semantics.

How bad is it really to either force initialization via a constructor, or get people to apply a builder pattern (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets too complex?  That's how this is often dealt with in language that don't have const.

Having an API that fundamentally doesn't expose mutation is arguably an even stronger statement of "don't change me" than annotating a type with constness. You also get separation of initialization/validation logic from the "meat of the class."  The worst case cost is a builder object.

Note that I'm not suggesting creating a bunch of "read-only interfaces" that everything inherits from.

Is this style deemed too annoying?

Yep, the builder pattern is roughly what Darin suggested.  As I noted earlier, that requires a lot more boilerplate, when I just want to write scoped_refptr<const Foo>.  Also, scoped_refptr<const Foo> makes it clear it is a reference to a const object.  Otherwise, we need a convention for the wrapper type to be named ImmutableFoo or ConstFoo something.

Clearly the builder pattern works.  But if scoped_refptr<const Foo> is not considered harmful, then it's unnecessary to force users to write extra code.

[Albert J. Wong (王重傑)]

On Thu, Sep 30, 2010 at 5:28 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Thu, Sep 30, 2010 at 5:10 PM, Marc-Antoine Ruel <mar...@google.com> wrote:

I'm fine with the change as I disagree that object deletion is object mutation.

Ok, thanks!

 

Ref counting is a way to emulate a heap with GC. In no way in languages with a GC the refcounting is part of the object. For example 'const' objects like strings in python/java/C# are still "refcounted". In C++ we tend to optimize refcounting by keeping it inside the object to save one pointer and one heap allocation.

Yeah, I think this is a case of an implementation detail leaking out into the interface.  shared_ptr doesn't have this restriction (and you don't have to have the separate heap allocation if you use make_shared).

 

In C++ const object references tend to be a pain on the long run, especially when it starts to involve virtual functions and members acting as a cache. But that's exactly to fix race conditions caused by this kind of design that one would like to enforce const reference.

I'm also fine with the other pattern to use const members and use a build factory when necessary.

So in short I don't care either way.

M-A

Le 30 septembre 2010 20:06, Albert J. Wong (王重傑) <ajw...@chromium.org> a écrit :

I don't think there's anything inherently damaging about having mutable, but the high situations described where this seems interesting largely revolves around wanting to create immutable objects.

However, I also have any discussion about alternatives while preserving the status quo ref counting semantics.

How bad is it really to either force initialization via a constructor, or get people to apply a builder pattern (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets too complex?  That's how this is often dealt with in language that don't have const.

Having an API that fundamentally doesn't expose mutation is arguably an even stronger statement of "don't change me" than annotating a type with constness. You also get separation of initialization/validation logic from the "meat of the class."  The worst case cost is a builder object.

Note that I'm not suggesting creating a bunch of "read-only interfaces" that everything inherits from.

Is this style deemed too annoying?

Yep, the builder pattern is roughly what Darin suggested.  As I noted earlier, that requires a lot more boilerplate, when I just want to write scoped_refptr<const Foo>.  Also, scoped_refptr<const Foo> makes it clear it is a reference to a const object.  Otherwise, we need a convention for the wrapper type to be named ImmutableFoo or ConstFoo something.

Couple of comments

First, I was suggesting we do _not_ use wrapper types. This gets old really fast.

Rather I was suggesting avoid having mutable objects at all in the cases it matters. This means you'd have 2 types, the "data object" and its associated "builder" or "factory" depending. You wouldn't need an "ImmutableFoo" or "ConstFoo" since there's no "Foo."

As for note the constness of reference object, it's doesn't seem that important if there are no APIs for mutation.  It may help readability for people who are glancing through the code, but I've found that in a system with bunches of immutable objects, you get a pretty quick feel for which ones are create-only.

But this is starting to get into a pick your preference discussion.

Clearly the builder pattern works.  But if scoped_refptr<const Foo> is not considered harmful, then it's unnecessary to force users to write extra code.

Yeah, in the end, this seems like the argument that holds the most weight.

-Albert

[William Chan (陈智昌)]

On Thu, Sep 30, 2010 at 6:31 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

On Thu, Sep 30, 2010 at 5:28 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Thu, Sep 30, 2010 at 5:10 PM, Marc-Antoine Ruel <mar...@google.com> wrote:

I'm fine with the change as I disagree that object deletion is object mutation.

Ok, thanks!

 

Ref counting is a way to emulate a heap with GC. In no way in languages with a GC the refcounting is part of the object. For example 'const' objects like strings in python/java/C# are still "refcounted". In C++ we tend to optimize refcounting by keeping it inside the object to save one pointer and one heap allocation.

Yeah, I think this is a case of an implementation detail leaking out into the interface.  shared_ptr doesn't have this restriction (and you don't have to have the separate heap allocation if you use make_shared).

 

In C++ const object references tend to be a pain on the long run, especially when it starts to involve virtual functions and members acting as a cache. But that's exactly to fix race conditions caused by this kind of design that one would like to enforce const reference.

I'm also fine with the other pattern to use const members and use a build factory when necessary.

So in short I don't care either way.

M-A

Le 30 septembre 2010 20:06, Albert J. Wong (王重傑) <ajw...@chromium.org> a écrit :

I don't think there's anything inherently damaging about having mutable, but the high situations described where this seems interesting largely revolves around wanting to create immutable objects.

However, I also have any discussion about alternatives while preserving the status quo ref counting semantics.

How bad is it really to either force initialization via a constructor, or get people to apply a builder pattern (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets too complex?  That's how this is often dealt with in language that don't have const.

Having an API that fundamentally doesn't expose mutation is arguably an even stronger statement of "don't change me" than annotating a type with constness. You also get separation of initialization/validation logic from the "meat of the class."  The worst case cost is a builder object.

Note that I'm not suggesting creating a bunch of "read-only interfaces" that everything inherits from.

Is this style deemed too annoying?

Yep, the builder pattern is roughly what Darin suggested.  As I noted earlier, that requires a lot more boilerplate, when I just want to write scoped_refptr<const Foo>.  Also, scoped_refptr<const Foo> makes it clear it is a reference to a const object.  Otherwise, we need a convention for the wrapper type to be named ImmutableFoo or ConstFoo something.

Couple of comments

First, I was suggesting we do _not_ use wrapper types. This gets old really fast.

Sorry, bad choice of words.  I understood what you meant.

 

Rather I was suggesting avoid having mutable objects at all in the cases it matters. This means you'd have 2 types, the "data object" and its associated "builder" or "factory" depending. You wouldn't need an "ImmutableFoo" or "ConstFoo" since there's no "Foo."

As for note the constness of reference object, it's doesn't seem that important if there are no APIs for mutation.  It may help readability for people who are glancing through the code, but I've found that in a system with bunches of immutable objects, you get a pretty quick feel for which ones are create-only.

But this is starting to get into a pick your preference discussion.

Clearly the builder pattern works.  But if scoped_refptr<const Foo> is not considered harmful, then it's unnecessary to force users to write extra code.

Yeah, in the end, this seems like the argument that holds the most weight.

Ok, if you agree with this, then I'm fine.  I am going to resist the urge to further debate the other issues since they're not as important :)

[Darin Fisher]

I'm convinced for what it's worth.  Thanks everyone for the discussion.

-Darin

[Iain Merrick]

It would be great to have a nice clear summary of this discussion posted somewhere.

A google search for "webkit refptr" gives me this excellent summary page: http://webkit.org/coding/RefPtr.html

It's much harder to find the equivalent guide for Chromium, if there is one. There are a couple of pages on C++ coding style but they mainly discuss code layout and inlining.

Cheers,

Iain

[Greg Thompson]

On Tue, Sep 28, 2010 at 6:34 PM, Peter Kasting <pkas...@chromium.org> wrote:

I'm in favor of supporting scoped_refptr<const X>.

On Tue, Sep 28, 2010 at 1:44 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

* Object deletion is basically object mutation.  If I pass const Foo* to a function, I don't really expect it to be deleted.

I've never really agreed with this.  First, it's not _inherently_ wrong to delete a pointer-to-const-object at some point, because

const int* x = new int;

delete x;

...doesn't warn.

On the off chance that anyone finds this dubious, I'll point out that not only does it not warn, it is explicitly valid (C++98 5.3.5.2):

"[Note: a pointer to a const type can be the operand of a delete-expression; it is not necessary to cast away the constness (5.2.11) of the pointer expression before it is used as the operand of the delete-expression. ]"

Greg

[Aaron Boodman]

On Thu, Sep 30, 2010 at 5:06 PM, Albert J. Wong (王重傑)
<ajw...@chromium.org> wrote:
> I don't think there's anything inherently damaging about having mutable, but
> the high situations described where this seems interesting largely revolves
> around wanting to create immutable objects.
> However, I also have any discussion about alternatives while preserving the
> status quo ref counting semantics.
> How bad is it really to either force initialization via a constructor, or
> get people to apply a builder pattern
> (http://en.wikipedia.org/wiki/Builder_pattern) if the initialization gets
> too complex?  That's how this is often dealt with in language that don't
> have const.
> Having an API that fundamentally doesn't expose mutation is arguably an even
> stronger statement of "don't change me" than annotating a type with
> constness. You also get separation of initialization/validation logic from
> the "meat of the class."  The worst case cost is a builder object.
> Note that I'm not suggesting creating a bunch of "read-only interfaces" that
> everything inherits from.
> Is this style deemed too annoying?

It seems we're already converging, but I just wanted to reply to this
comment. I think it is unfortunate to force an entire class to be
immutable when you really only want some instances of the class to be.
The resulting class is much less flexible.

One place where the flexibility is nice is unit testing. With the
builder+class-lacking-setters pattern, a new instance of the class
must be built for each configuration you want to test. I think this
might tend to discourage extensive testing.

I guess this is basically the "too much work" argument, but I wanted
to point out that it manifests in multiple places.

Thanks everyone, this has been an interesting discussion to follow.

- a