proposal: allow std::shared_ptr and std::unique_ptr

[Matthew Dempsky]

I'd like to convert sandbox/linux to use std::shared_ptr and std::unique_ptr to remove some of its dependencies on base, and make it easier to reuse outside of Chromium.

The Google C++ style guide already allows and encourages use of these classes, and my impression is base's scoped_ptr is intended to be replaced by std::unique_ptr.

The situation for std::shared_ptr in Chromium seems less clear (intrusive vs non-intrusive ref counting), but at least within sandbox/linux the uses of scoped_refptr are all internal / implementation details that don't affect the end user API.

[Ryan Hamilton]

+1 for std::unique_ptr. QUIC and SPDY code is shared with an internal repository which uses std::unique_ptr. Being able to use std::unique_ptr in the Chromium code would result in fewer differences between the copies of the code.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF52%2BS4x0qQS1zvuJV8%3D5KqSG4R-wQV9DHhNS%3Dcz1SnQg9jj-A%40mail.gmail.com.

[Daniel Cheng]

I think we should consider std::shared_ptr separately.

I had a draft for std::unique_ptr, but since the thread already started, I'll just dump it here instead. std::unique_ptr is blocked on std::move(), since it's not as useful without it. A few things that I think are worth highlighting:

• converting between std::unique_ptr and scoped_ptr: in order to allow people to start using it incrementally, it's probably useful to have a way to convert back and forth between them. https://codereview.chromium.org/1432193002 is a proposal that would allow this.
• std::unique_ptr and base::Bind: base::Bind() and base::Passed() depends on the custom macros for move semantics, which add a Pass() member to movable types. One option is to use template metaprogramming magic to make base::Passed() magically use std::move() for types with move constructors/operators. Another is to add more magic to base::Bind() so it can implicitly convert std::unique_ptr to scoped_ptr.
  
• searching a container of unique_ptrs: since unique_ptr<T> is not comparable to T*, code like this won't work:
      std::set<std::unique_ptr<T>> s;
      std::unique_ptr<T> p(new T);
      T* raw_p = p.get();
      s.insert(std::move(p));
      auto it = s.find(raw_p); // error: unique_ptr<T> is not comparable to T*
  Instead, you'll have to write this:
      auto it = std::find_if(s.begin(), s.end(),
          [raw_p](const std::unique_ptr<T>& x) {
            return x.get() == raw_p;
          }); // O(n) instead of O(lg n)
• specializations of std::default_delete: currently, there are some specializations of DefaultDeleter so code doesn't have to specify a custom deleter when instantiating a scoped_ptr for that type [1]. User code isn't usually allowed to do random things in namespace std (the spec uses the word "undefined behavior" a lot here), but there's an exception for specializations in 17.6.4.2.1 and I believe this rule would apply to std::default_delete. It might be nice to have some explicit guidance around this.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJ_4DfR8G9z6kiVuq3BUHZGLD1fgXrrMTZgoAFNmBOPnVN3FGA%40mail.gmail.com.

[Daniel Cheng]

Oh, one other thing I forgot to mention: scoped_ptr<T> currently has make_scoped_ptr. For unique_ptr, there are several options:

1. Add a WrapUnique helper:
       auto p = WrapUnique(new RenderWidgetHostViewDelegateClientFactoryHelper(delegate, client));
   
   Why not call it make_unique_ptr? As it turns out, C++14 already has std::make_unique which has different semantics. This leads us to option...
   
   
2. Add a MakeUnique helper that does the same thing as C++14's std::make_unique.
       auto p = MakeUnique<RenderWidgetHostViewDelegateClientFactoryHelper>(delegate, client);
   
   
3. Do both (incidentally, this is what google3 did).
   
   
4. Do neither.

(This is assuming we allow auto to be used in this way.)

Daniel 

[Dana Jansens]

On Thu, Nov 12, 2015 at 2:08 PM, Daniel Cheng <dch...@chromium.org> wrote:

Oh, one other thing I forgot to mention: scoped_ptr<T> currently has make_scoped_ptr. For unique_ptr, there are several options:

1. Add a WrapUnique helper:
       auto p = WrapUnique(new RenderWidgetHostViewDelegateClientFactoryHelper(delegate, client));
   
   Why not call it make_unique_ptr? As it turns out, C++14 already has std::make_unique which has different semantics. This leads us to option...
   
   
2. Add a MakeUnique helper that does the same thing as C++14's std::make_unique.
       auto p = MakeUnique<RenderWidgetHostViewDelegateClientFactoryHelper>(delegate, client);
   

I'd prefer to be as close to the future-standard as possible. Just as any differences in scoped_ptr vs unique_ptr required us to write more complicated patches to convert things (in that case to avoid causing crashes or bugs), I'd also want things we do that will be replaced by C++14 to be as search-and-replaceable as possible. So I like base::MakeUnique better than base::WrapUnique.

The latter is of course closer to what we have with scoped_ptr, so it makes us write more patches up front now, instead of later.. but the incentive to do so to use std::unique_ptr is probably higher than the incentive to use std::make_unique, leaving us hopefully less likely to end up with both std::make_unique and base::something for an indefinitely long time.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqFW91hRr1smPnDjJbxPF9YSFqFnrRy9C9as%2BL%2BavD9Hg%40mail.gmail.com.

[Nico Weber]

I too think it makes sense to handle unique_ptr and shared_ptr separately.

Allowing unique_ptr early makes sense to me (for quic if nothing else), and https://codereview.chromium.org/1432193002/ seems like a good way to get there.

WrapUnique() also sounds good to me. Is it feasible to mass-replace "make_scoped_ptr" to "wrap_scoped_ptr" to keep names similar? (/me shakes fist at standard for giving make_unique gratuitously different behavior than make_pair etc).

I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to have some story around these two classes like we do for unique_ptr / scoped_ptr.

On Thu, Nov 12, 2015 at 2:08 PM, Daniel Cheng <dch...@chromium.org> wrote:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqFW91hRr1smPnDjJbxPF9YSFqFnrRy9C9as%2BL%2BavD9Hg%40mail.gmail.com.

[Jeffrey Yasskin]

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
> I too think it makes sense to handle unique_ptr and shared_ptr separately.

+1.

> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to
> have some story around these two classes like we do for unique_ptr /
> scoped_ptr.

There are some efficiency differences between shared_ptr and
scoped_refptr, which should be explored in the proposal to allow
shared_ptr. e.g. sizeof(shared_ptr)>sizeof(scoped_refptr), and
enable_shared_from_this also has overhead.

> https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiG%2B2x5ruj%2B%2B7ti8gdddtzbdvqwKzwEoVLd6hf2kz7J%2BHA%40mail.gmail.com.

[Dana Jansens]

On Thu, Nov 12, 2015 at 2:28 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
> I too think it makes sense to handle unique_ptr and shared_ptr separately.

+1.

> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to
> have some story around these two classes like we do for unique_ptr /
> scoped_ptr.

There are some efficiency differences between shared_ptr and
scoped_refptr, which should be explored in the proposal to allow
shared_ptr. e.g. sizeof(shared_ptr)>sizeof(scoped_refptr), and
enable_shared_from_this also has overhead.

Yah. My general feelings are that:

1) I don't like using types in std:: that provide the same things as types in base::. Using different "standardish" types in different places feels awkward and confusing.

2) I prefer using types in std:: to having our own implementation of it in base:: when possible.

All that is to say, I don't feel excited about allowing std::shared_ptr while we also have scoped_refptr.

And I enthusiastically welcome data-driven proposals around replacing scoped_refptr with std::shared_ptr if this turns out to be reasonable.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANh-dXntiTqm%2Baq6RkC9To5C4hrOwWAZqc-u1rscVjLBDz_gHg%40mail.gmail.com.

[Vladimir Levin]

On Thu, Nov 12, 2015 at 2:35 PM, 'Dana Jansens' via cxx <c...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 2:28 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
> I too think it makes sense to handle unique_ptr and shared_ptr separately.

+1.

> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to
> have some story around these two classes like we do for unique_ptr /
> scoped_ptr.

There are some efficiency differences between shared_ptr and
scoped_refptr, which should be explored in the proposal to allow
shared_ptr. e.g. sizeof(shared_ptr)>sizeof(scoped_refptr), and
enable_shared_from_this also has overhead.

Yah. My general feelings are that:

1) I don't like using types in std:: that provide the same things as types in base::. Using different "standardish" types in different places feels awkward and confusing.

2) I prefer using types in std:: to having our own implementation of it in base:: when possible.

All that is to say, I don't feel excited about allowing std::shared_ptr while we also have scoped_refptr.

And I enthusiastically welcome data-driven proposals around replacing scoped_refptr with std::shared_ptr if this turns out to be reasonable.

I agree with this. I would say the biggest problem I have with scoped_refptr is that the class has to be declared as being ref counted, instead of the ref counting being external (as is the case in shared_ptr). However, I also realize that there are efficiency benefits to scoped_refptr that shared_ptr can't provide. So, we probably need data.

+1 to unique_ptr though. One thing to note is that shared_ptr can be constructed from a unique_ptr implicitly, so we have to be careful not to accidentally say "shared_ptr" somewhere.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRQdfH0u-CeHsQSL9NJUuQerz_8-p%2Bwb-H405o0Y1%3D%2B_g%40mail.gmail.com.

[Matthew Dempsky]

On Thu, Nov 12, 2015 at 2:35 PM, Dana Jansens <dan...@google.com> wrote:

All that is to say, I don't feel excited about allowing std::shared_ptr while we also have scoped_refptr.

I agree we should have a recommended default.  I'm even fine with std::shared_ptr being discouraged in general.  I'd still like to be able to use it in parts of the tree that are intended to be reused outside of Chromium without depending on base.

E.g., I see in third_party/webrtc, there's a bunch of comments like "// TODO(solenberg): Change this to a shared_ptr once we can use C++11."  Is it okay for third_party/webrtc to use std::shared_ptr, or will they need to continue using their own rtc::scoped_refptr class?

Analogously, is it okay to use std::shared_ptr just as an implementation detail of sandbox/linux?  Would the answer change if we rename that directory to third_party/bauxite?  (Bauxite is already the name of the project to make sandbox/linux work outside of Chromium.)

[Nico Weber]

I think we should have a single guideline for stuff in src.git, else things will get super confusing.

[Matthew Dempsky]

On Thu, Nov 12, 2015 at 3:13 PM, Nico Weber <tha...@chromium.org> wrote:

I think we should have a single guideline for stuff in src.git, else things will get super confusing.

So we need to split sandbox/linux into bauxite.git to be able to use std::shared_ptr?

[Daniel Cheng]

Forking this since this discussion is largely about shared_ptr.

I think if sandbox/linux were to move into //third_party/bauxite, then it could probably do whatever it wants. But inside Chromium, shared_ptr feels largely redundant with scoped_refptr (not to mention we also have WTF::RefPtr).

Daniel  

[Nico Weber]

I agree with this. "If it's in chromium's repo, it follows Chromium's rules" is a simple rule that's easy to understand. I think it'd be nice for chromium-affine projects to do so too, but that's already not the case (webrtc, skia, …).

It also imposes some cost on deciding to do your own thing, which is probably a good incentive.

(Note that stuff outside of src isn't allowed to depend on base/; some bits of sandbox/linux do depend on base/ so I suppose these bits wouldn't be pulled out.)

[Brett Wilson]

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:

I too think it makes sense to handle unique_ptr and shared_ptr separately.

Allowing unique_ptr early makes sense to me (for quic if nothing else), and https://codereview.chromium.org/1432193002/ seems like a good way to get there.

WrapUnique() also sounds good to me. Is it feasible to mass-replace "make_scoped_ptr" to "wrap_scoped_ptr" to keep names similar? (/me shakes fist at standard for giving make_unique gratuitously different behavior than make_pair etc).

I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to have some story around these two classes like we do for unique_ptr / scoped_ptr.

shared_ptr is the same base base::linked_ptr as far as I understand it.

I believe almost all uses of base::linked_ptr can be deleted and replaced with movable types, since the normal use for this was to be able to put objects in standard containers without copying or using raw pointers. So I would advocate doing this replacement rather than using base::shared_ptr in this case.

My gut response is that I don't like shared_ptr and I'd prefer not to allow it now. One big thing we do with our RefCounted class is allow it to express which thread it should be deleted on. This is pretty important to us, so I don't think we can remove our own RefCounted class. And I think the different models about ownership will get confusing if we have both scoped_ptr and shared_ptr.

Brett

[Peter Kasting]

On Thu, Nov 12, 2015 at 3:16 PM, Daniel Cheng <dch...@chromium.org> wrote:

Forking this since this discussion is largely about shared_ptr.

Brett responded about shared_ptr directly in the original thread, but I'm moving my response to him here to try and keep shared_ptr stuff in this thread.

On Thu, Nov 12, 2015 at 3:52 PM, Brett Wilson <bre...@chromium.org> wrote:

My gut response is that I don't like shared_ptr and I'd prefer not to allow it now. One big thing we do with our RefCounted class is allow it to express which thread it should be deleted on. This is pretty important to us, so I don't think we can remove our own RefCounted class.

I worry a bit about code that does this, though.  In most of the cases I've seen, use of this functionality actually means that the code isn't sufficiently robust against some set of circumstances, and Bad Things can happen.  (I'm being vague because it's been a little while so I can't remember the details of specific examples.)

I'm of two minds here.  On one hand, I would really prefer to use the STL if at all possible and avoid our own classes that basically tackle the same problem.  For this reason, I really would prefer shared_ptr over scoped_refptr etc. as long as there's not a significant performance loss.

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

PK

[Jeffrey Yasskin]

On Thu, Nov 12, 2015 at 3:52 PM, Brett Wilson <bre...@chromium.org> wrote:

> On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
>>
>> I too think it makes sense to handle unique_ptr and shared_ptr separately.
>>

>> Allowing unique_ptr early makes sense to me (for quic if nothing else),
>> and https://codereview.chromium.org/1432193002/ seems like a good way to get
>> there.
>>
>> WrapUnique() also sounds good to me. Is it feasible to mass-replace
>> "make_scoped_ptr" to "wrap_scoped_ptr" to keep names similar? (/me shakes
>> fist at standard for giving make_unique gratuitously different behavior than
>> make_pair etc).
>>

>> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need
>> to have some story around these two classes like we do for unique_ptr /
>> scoped_ptr.
>
>

> shared_ptr is the same base base::linked_ptr as far as I understand it.

~linked_ptr() takes O(refcount) time, while ~shared_ptr is O(1).
linked_ptr isn't thread-safe, while shared_ptr is. linked_ptr doesn't
require allocating a control block, while shared_ptr does (although
make_shared can allocate the control block adjacent to the object).
linked_ptr doesn't allow weak references, while shared_ptr does.
Otherwise, sure, they're both shared ownership mechanisms.

> My gut response is that I don't like shared_ptr and I'd prefer not to allow
> it now. One big thing we do with our RefCounted class is allow it to express
> which thread it should be deleted on. This is pretty important to us, so I

> don't think we can remove our own RefCounted class. And I think the
> different models about ownership will get confusing if we have both
> scoped_ptr and shared_ptr.

shared_ptr() has a deleter argument that can be used to pick the
destruction thread:
http://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr

The main difference here is again that RefCountedThreadSafe requires a
single behavior for a whole type, while shared_ptr works per-instance.

I suspect we should stick with RefCounted*, but please lets dismiss
shared_ptr for real reasons and not because we don't understand it.

Jeffrey

[Brett Wilson]

Yeah, philosophically, I think it's important to be able to know by looking at the class what type of memory management it expects.

As an example, one of the big things we use RefCounted for is for posting work to a background thread (among other options for using Bind). But if this became shared_ptr to keep the object alive across the post, it becomes too easy to screw up the memory management and using both manual deleting + shared_ptr (if my understanding of shared_ptr is correct. Our RefCounted class has a lot of protections that tell you right away when you screw up.

Brett

[Dana Jansens]

On Thu, Nov 12, 2015 at 3:14 PM, Matthew Dempsky <mdem...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 3:13 PM, Nico Weber <tha...@chromium.org> wrote:

I think we should have a single guideline for stuff in src.git, else things will get super confusing.

So we need to split sandbox/linux into bauxite.git to be able to use std::shared_ptr?

I would prefer that also, until/unless we decide to use it everywhere. Per-module style guide rules seems like a very slippery slope to spaghetti code.

[Nico Weber]

On Thu, Nov 12, 2015 at 1:15 PM, Daniel Cheng <dch...@chromium.org> wrote:

I think we should consider std::shared_ptr separately.

I had a draft for std::unique_ptr, but since the thread already started, I'll just dump it here instead. std::unique_ptr is blocked on std::move(), since it's not as useful without it. A few things that I think are worth highlighting:

• converting between std::unique_ptr and scoped_ptr: in order to allow people to start using it incrementally, it's probably useful to have a way to convert back and forth between them. https://codereview.chromium.org/1432193002 is a proposal that would allow this.
• std::unique_ptr and base::Bind: base::Bind() and base::Passed() depends on the custom macros for move semantics, which add a Pass() member to movable types. One option is to use template metaprogramming magic to make base::Passed() magically use std::move() for types with move constructors/operators. Another is to add more magic to base::Bind() so it can implicitly convert std::unique_ptr to scoped_ptr.
  

(IIRC this doesn't work well because move constructors don't guarantee to zero out the moved-from object. It's true for scoped_ptr and I think unique_ptr and hopefully our move-emulated objects we currently have, but not for all movable types in general.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKr3LT1xQT9t6aMbKepPRb6UbGUy6rpfomLG6QEhMHQJhA%40mail.gmail.com.

[James Robinson]

On Thu, Nov 12, 2015 at 5:49 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 1:15 PM, Daniel Cheng <dch...@chromium.org> wrote:

I think we should consider std::shared_ptr separately.

I had a draft for std::unique_ptr, but since the thread already started, I'll just dump it here instead. std::unique_ptr is blocked on std::move(), since it's not as useful without it. A few things that I think are worth highlighting:

• converting between std::unique_ptr and scoped_ptr: in order to allow people to start using it incrementally, it's probably useful to have a way to convert back and forth between them. https://codereview.chromium.org/1432193002 is a proposal that would allow this.
• std::unique_ptr and base::Bind: base::Bind() and base::Passed() depends on the custom macros for move semantics, which add a Pass() member to movable types. One option is to use template metaprogramming magic to make base::Passed() magically use std::move() for types with move constructors/operators. Another is to add more magic to base::Bind() so it can implicitly convert std::unique_ptr to scoped_ptr.
  

(IIRC this doesn't work well because move constructors don't guarantee to zero out the moved-from object. It's true for scoped_ptr and I think unique_ptr and hopefully our move-emulated objects we currently have, but not for all movable types in general.)

The moved-from state for std::unique_ptr and for base' scoped_ptr is defined to be nullptr (c++11 20.7.1.4 for std::unique_ptr).  In general the moved-from state is not guaranteed to be anything in particular except that it will always be valid (i.e. safe to assign to or destroy).

- James

[Vladimir Levin]

On Thu, Nov 12, 2015 at 4:13 PM, 'Brett Wilson' via cxx <c...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 4:07 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Nov 12, 2015 at 3:16 PM, Daniel Cheng <dch...@chromium.org> wrote:

Forking this since this discussion is largely about shared_ptr.

Brett responded about shared_ptr directly in the original thread, but I'm moving my response to him here to try and keep shared_ptr stuff in this thread.

On Thu, Nov 12, 2015 at 3:52 PM, Brett Wilson <bre...@chromium.org> wrote:

My gut response is that I don't like shared_ptr and I'd prefer not to allow it now. One big thing we do with our RefCounted class is allow it to express which thread it should be deleted on. This is pretty important to us, so I don't think we can remove our own RefCounted class.

I worry a bit about code that does this, though.  In most of the cases I've seen, use of this functionality actually means that the code isn't sufficiently robust against some set of circumstances, and Bad Things can happen.  (I'm being vague because it's been a little while so I can't remember the details of specific examples.)

I'm of two minds here.  On one hand, I would really prefer to use the STL if at all possible and avoid our own classes that basically tackle the same problem.  For this reason, I really would prefer shared_ptr over scoped_refptr etc. as long as there's not a significant performance loss.

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

Yeah, philosophically, I think it's important to be able to know by looking at the class what type of memory management it expects.

FWIW, there are other ways to achieve this. For example, 

class Foo {

 public:

   static std::shared_ptr<Foo> Create();

   ...

 private:

   Foo();

   ...

};

This would say to me that this class expects to be shared.  That is to say, we can still make sure that classes that must be ref counted can indicate this. However, the difference with a shared_ptr is that it's much harder to enforce that every class that is going to be used in a shared_ptr must mark itself as such. Is the latter part as important?

To put it differently, if a class has a static create that instead returns a std::unique_ptr<Foo>, then it could still be used in a shared_ptr. Is this going to cause bugs? If so then maybe we can hold off on shared_ptr (and do you have examples of where that might be a problem? :) )

As a separate point, we also have some code that starts out with a scoped_refptr, then passes a raw pointer down somewhere and whatever class receives the raw pointer puts it in another scoped_refptr. That's fine when ref counting is done in the class, but with shared_ptrs, if I'm not mistaken, this is going to cause a double free, since two blocks of ref counting would be allocated. I think if we lean towards allowing shared_ptr, then we at least need an audit of current usages first.

As an example, one of the big things we use RefCounted for is for posting work to a background thread (among other options for using Bind). But if this became shared_ptr to keep the object alive across the post, it becomes too easy to screw up the memory management and using both manual deleting + shared_ptr (if my understanding of shared_ptr is correct. Our RefCounted class has a lot of protections that tell you right away when you screw up.

Brett

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV9Vnsi5sgMo5kfEcgT9Tku4r5cxD6uCN4QCPZ_HhpY%2BeA%40mail.gmail.com.

[Daniel Cheng]

Is allowing this to work with real movable types any worse than what we have today? You can already run a Callback with a move-only emulation type more than once. Doing so with the current system is probably a programmer error, and would remain so with real movable types.

Daniel

 

- James

[Dana Jansens]

I'd like to be precise in our terminology here. Callback will not do anything but copy _movable_ types. It will only call .Pass() (aka move()) on _move-only_ types that were made with our move.h macro (cuz it makes them move-only).

I agree that you can do so more than once today, which is a programmer error. There are relatively few move-only types in Chromium compared to types in the standard library, and probably even fewer are passed through Callbacks. Hopefully they would cause an obvious crash by nulling things instead of swapping things in the Move constructor, but of course there's no guarantees.

Using Callback with std::unique_ptr would be "okay" and fail in the same ways that scoped_ptr would today. But it won't compile without changing Callback because it would try to copy it.

I think writing Passed support for unique_ptr that converts to a scoped_ptr seems okay because that's just basically a spelling change for people using it. I do not support adding general move-only or move-able support to base::Callback as it is today (copyable + repeat-callable) and think that we need to revisit those things in implementing support for general move-only types (outside of our special macro).

Which leads to temporary guidance probably being something like, if you want to pass a move-only type through a callback, you need to wrap it in a std::unique_ptr. Perhaps until Bind/Callback is fixed, it would be nice to actually migrate other move-only types off of the base/move.h macro, which would cause them to require wrapping in a scoped_ptr/unique_ptr to go through Callback. Then we have some really well-defined behaviour there (definitely a nullptr crash on misuse) until we make things better.

So.. all of this is to say, I do not think we should block unique_ptr on changing Callback, people can convert before calling Passed if they need to. And I do think we should make implicit conversions to scoped_ptr inside the Callback system for now so people can do this transition more seamlessly as well.

Because scoped_ptr and unique_ptr are so alike, I think that discussions about general move-only or movable types and Callback probably shouldn't guide the decision about how or where to use std::unique_ptr in our codebase.

 

Daniel

 

- James

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrbhiX9knK%2BJzK3A46Jbup0RwZa2rW10Bc7XY4Ca2fc2g%40mail.gmail.com.

[Chris Blume]

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

I agree with discouraging ref counting in general. And I understand that making ref counting more difficult can be a very very good way to discourage it. However, I strongly dislike the idea of any type not related to memory management having to deal with memory management. Especially when it involves inheritance. Imagine if you could not put an int on the stack because it was ref counted in some case. So instead either all ints are ref counted or we have two types (one for ref counted and one not). Neither of these options is good and it stems from the type assuming too much about its usage. Let the type care only about the one thing it should care about. Compositing restrictions for each usage is much easier (I need an int and I need it ref counted).

Given these two evils, I feel like shared_ptr is the lesser.

 

As a separate point, we also have some code that starts out with a scoped_refptr, then passes a raw pointer down somewhere and whatever class receives the raw pointer puts it in another scoped_refptr. That's fine when ref counting is done in the class, but with shared_ptrs, if I'm not mistaken, this is going to cause a double free, since two blocks of ref counting would be allocated. I think if we lean towards allowing shared_ptr, then we at least need an audit of current usages first.

This sounds to me like a problem that should be fixed.

It sounds like it happens to work now because the type has the ref count baked in. But the design requires it be ref counted and doesn't explicitly state that. It relies on the type to enforce that.

Since the design requires the type be ref counted, it could explicitly state that by taking a shared_ptr instead of a raw pointer.

[Vladimir Levin]

On Wed, Nov 18, 2015 at 6:23 AM, Chris Blume <cbl...@google.com> wrote:

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

I agree with discouraging ref counting in general. And I understand that making ref counting more difficult can be a very very good way to discourage it. However, I strongly dislike the idea of any type not related to memory management having to deal with memory management. Especially when it involves inheritance. Imagine if you could not put an int on the stack because it was ref counted in some case. So instead either all ints are ref counted or we have two types (one for ref counted and one not). Neither of these options is good and it stems from the type assuming too much about its usage. Let the type care only about the one thing it should care about. Compositing restrictions for each usage is much easier (I need an int and I need it ref counted).

Given these two evils, I feel like shared_ptr is the lesser.

+1, I very much dislike having to decide whether a class should be ref counted or not when writing the class itself, instead of when deciding (or changing) its usage later. 

 

 

As a separate point, we also have some code that starts out with a scoped_refptr, then passes a raw pointer down somewhere and whatever class receives the raw pointer puts it in another scoped_refptr. That's fine when ref counting is done in the class, but with shared_ptrs, if I'm not mistaken, this is going to cause a double free, since two blocks of ref counting would be allocated. I think if we lean towards allowing shared_ptr, then we at least need an audit of current usages first.

This sounds to me like a problem that should be fixed.

It sounds like it happens to work now because the type has the ref count baked in. But the design requires it be ref counted and doesn't explicitly state that. It relies on the type to enforce that.

Since the design requires the type be ref counted, it could explicitly state that by taking a shared_ptr instead of a raw pointer.

I agree, I was only pointing it out that this exists and we have to be careful not to just s/scoped_refptr/shared_ptr/.  

[Ryan Sleevi]

On Wed, Nov 18, 2015 at 10:59 AM, 'Vladimir Levin' via cxx <c...@chromium.org> wrote:

+1, I very much dislike having to decide whether a class should be ref counted or not when writing the class itself, instead of when deciding (or changing) its usage later. 

Given the sheer number of reference counting issues I've seen - either through incoming reviews or bugs that make it out - I would argue that "deciding whether a class should be ref-counted or not" is very much a decision you should be making up front, at the design time, because it materially affects everything around it.

Switching to reference counting later is almost _always_ an anti-pattern, and using reference counting early in your design is... well, almost always an anti-pattern too :)

At the risk of "+1/-1" wars, this is as much an intentional design decision as could be, no different than deciding whether or not you want to allow inheritance, whether to make your class concrete or an interface, whether or not it's a 'public' or 'private' API, and all of the other attendant software design issues.

[Brett Wilson]

Yea, I fear having every individual programmer decide at time of use whether a given class should be refcounted or not will be disastrous. I think reference counting generally works OK in Chrome today.

Brett

[Vladimir Levin]

On Wed, Nov 18, 2015 at 11:06 AM, Brett Wilson <bre...@google.com> wrote:

On Wed, Nov 18, 2015 at 11:03 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Nov 18, 2015 at 10:59 AM, 'Vladimir Levin' via cxx <c...@chromium.org> wrote:

+1, I very much dislike having to decide whether a class should be ref counted or not when writing the class itself, instead of when deciding (or changing) its usage later. 

Given the sheer number of reference counting issues I've seen - either through incoming reviews or bugs that make it out - I would argue that "deciding whether a class should be ref-counted or not" is very much a decision you should be making up front, at the design time, because it materially affects everything around it.

I assume these bugs were using our current ref counting methods? If that's the case, then maybe it's a case for changing the current system. :) On a more serious note, I agree that ref counting is hard and it should be used rarely and carefully. I just feel that baking that knowledge into the class at the get-go is not good. If someone decides that a class has to be ref counted, that forces all callers to use refcounting. Even if those callers would otherwise prefer to create a scoped_ptr instead.

 

Switching to reference counting later is almost _always_ an anti-pattern, and using reference counting early in your design is... well, almost always an anti-pattern too :)

At the risk of "+1/-1" wars, this is as much an intentional design decision as could be, no different than deciding whether or not you want to allow inheritance, whether to make your class concrete or an interface, whether or not it's a 'public' or 'private' API, and all of the other attendant software design issues.

Yea, I fear having every individual programmer decide at time of use whether a given class should be refcounted or not will be disastrous. I think reference counting generally works OK in Chrome today.

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

As mentioned earlier, the class itself can guide (or rather force) the caller by exposing a create function that returns a shared_ptr (which I think is pretty important when the class is using shared_from_this). 

The problem, as cblume mentioned, is that if there are situations where you want one object not to ref count and the other one to ref count. Right now that's only possible with necessarily ref counting and having a ref count of 1. It works, but it doesn't specify intent very well. (As a disclaimer, I don't actually have specific examples of these situations)

 

Brett

[Peter Kasting]

On Wed, Nov 18, 2015 at 6:23 AM, Chris Blume <cbl...@google.com> wrote:

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

I agree with discouraging ref counting in general. And I understand that making ref counting more difficult can be a very very good way to discourage it. However, I strongly dislike the idea of any type not related to memory management having to deal with memory management. Especially when it involves inheritance. Imagine if you could not put an int on the stack because it was ref counted in some case. So instead either all ints are ref counted or we have two types (one for ref counted and one not). Neither of these options is good and it stems from the type assuming too much about its usage. Let the type care only about the one thing it should care about. Compositing restrictions for each usage is much easier (I need an int and I need it ref counted).

I think the choice of example type here is misleading.  An int is both conceptually simple and hugely broad in its applicability.  All kinds of code wants to use ints for all kinds of reasons, but what they actually do with them -- store a single value and retrieve it -- is very simple.

Refcounted classes in Chromium are almost always at the other end of those scales.  They hold complex functionality and they are usually used in a limited set of circumstances.  Being refcounted often affects class design choices like how deletion happens or what needs locking.  For all these reasons, it generally makes sense for the question of refcounting to not be decoupled from the class design.

Now, in a few cases I might agree with you.  Let's say we wanted a class to represent a complex number (like std::complex).  That's probably a class where refcounting decisions can safely be made outside the class.  But I don't believe most Chromium code is like that.

PK

[Ruud van Asseldonk]

Refcounted classes in Chromium are almost always at the other end of those scales.  They hold complex functionality and they are usually used in a limited set of circumstances.  Being refcounted often affects class design choices like how deletion happens or what needs locking.  For all these reasons, it generally makes sense for the question of refcounting to not be decoupled from the class design.

To give a concrete example: here an instance could have been stack allocated perfectly well, but because the class is refcounted it now incurs a heap allocation.

[Peter Kasting]

On Wed, Nov 18, 2015 at 11:34 AM, Ruud van Asseldonk <ru...@google.com> wrote:

Refcounted classes in Chromium are almost always at the other end of those scales.  They hold complex functionality and they are usually used in a limited set of circumstances.  Being refcounted often affects class design choices like how deletion happens or what needs locking.  For all these reasons, it generally makes sense for the question of refcounting to not be decoupled from the class design.

To give a concrete example: here an instance could have been stack allocated perfectly well, but because the class is refcounted it now incurs a heap allocation.

I don't know anything about TraceValue and why ConvertibleToTraceFormat is RefCounted, so I can't speak concretely about this case.

I'm certain there are cases in Chromium today where we refcount needlessly and it would have been safe to not refcount.  That's why I was careful to use words like "generally" when making my argument.

PK 

[Ryan Sleevi]

On Wed, Nov 18, 2015 at 11:20 AM, Vladimir Levin <vmp...@google.com> wrote:

I assume these bugs were using our current ref counting methods? If that's the case, then maybe it's a case for changing the current system. :)

No. Lifetimes are hard - reference counting expressly serves to attempt to decouple sane lifetime management.

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyRJcpJsUJ , while being about WeakPtr, certainly discusses a lot of the challenges in lifetime management and coupling.

Also, https://groups.google.com/a/chromium.org/d/msg/chromium-dev/BLBuqeq29T4/Lm2xym_IYxcJ

While I'm totally in favour of loosening coupling when possible, we have to be careful that when things are strongly coupled, we actually loosen coupling, rather than just obscure coupling. Reference counting is one way in which coupling is obscured, not lessened.

 

On a more serious note, I agree that ref counting is hard and it should be used rarely and carefully. I just feel that baking that knowledge into the class at the get-go is not good. If someone decides that a class has to be ref counted, that forces all callers to use refcounting.

Correct. Which

1) Forces careful consideration as to the lifetimes of both objects (conceptually) and instances (as concrete things with lifetime management)

2) Is a net-negative enough to discourage reference counting.

 

Even if those callers would otherwise prefer to create a scoped_ptr instead.

 

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

In practice, this tends to work out extremely poorly.

A classic example it's let's imagine

class Foo {

 public:

  Foo(Bar* bar, Baz* baz);

};

This tells you some things about lifetime management, but not everything.

The immediate read should be that "Bar and Baz should remain valid for at least as long as the constructor, but likely as long as the object"

If that was perhaps implicitly guaranteed in some way - for example, if Baz was having ownership transferred, your reviewer should push on you to make that clearer:

Foo(Bar* bar, scoped_ptr<Baz> baz);

 

But we at least know that absent other information, Bar MUST remain valid for the duration of Foo. There's a tight coupling of those lifetimes.

Now, regardless of whether you're using RefCounted or shared_ptr<>, the only way to safely use 'foo' is to ensure that everything that may take a reference to Foo outlives Bar. That's hard. Hard for people and hard for machines to guarantee - and that's why reference counting is extremely dangerous.

The current pattern - deriving from RefCounted - makes this somewhat easier. This is because you MUST make a decision, at design time, whether or not Foo should be refcounted. If it is, that immediately forces Foo to be un-stack-allocatable (we have a clang plugin to make sure no public dtors exist throughout the inheritance chain of Foo - or anything that implements Foo - to ensure this). This means that everyone _must_ write something like

scoped_refptr<Foo> foo(new Foo(bar));

And that's a big sign, at time of writing the code, that Foo/Bar are quite coupled now. It should hopefully raise red flags - and minimally raise the standard - to make sure at all places that "foo" (the instance) outlives "bar".

Now, let's say we embraced shared_ptr, which is effectively "recipient decides" rather than "creator decides" reference counting. Let's say Foo is

class Foo {

 public:

  explicit Foo(Bar* bar);

 private:

  shared_ptr<Bar> bar;

};

From reading the constructor, one would again presume the lifetime guarantees - that Bar (and anything Bar depends on) must outlive Foo. Except now the contract has been changed - Bar (and dependents) doesn't just have Foo, they have to outlive everything that might have taken the shared_ptr.

If Foo ever gives out the shared_ptr<> to 'something' (which is inevitably why you make it shared - so you can share), then Foo no longer can make any guarantees as to the lifetime of Bar, nor can anyone writing Bar (such as adding a new dependency for Bar) know what the lifetimes or risks are.

Lifetime issues are subtle and hard to trace. shared_ptr<>, while appealing in many ways, can further obfuscate the lifetimes and guarantees - which makes it easier to write buggy software. RefCounted is not perfect in this regard - the fact that anyone can grab the reference to the object (RefCounted has an implied enable_shared_from_this) makes it complicated, and Blink's types (RefPtr vs OwnRefPtr, etc) try to solve _that_ problem as well.

Perfect? No. But certainly why reference counting is hard :)

As mentioned earlier, the class itself can guide (or rather force) the caller by exposing a create function that returns a shared_ptr (which I think is pretty important when the class is using shared_from_this). 

The problem, as cblume mentioned, is that if there are situations where you want one object not to ref count and the other one to ref count. Right now that's only possible with necessarily ref counting and having a ref count of 1.

Well, no, there's RefCountedData, which conceptually matches shared_ptr in that's an external ref count rather than an intrusive reference count.

 

It works, but it doesn't specify intent very well. (As a disclaimer, I don't actually have specific examples of these situations)

I have yet to see such a situation in practice come up which didn't already have other design issues (either in lifetimes or in coupling). That's anecdotal, for sure, but sometimes the best way to make progress is with concrete use cases. We have plenty of concrete evidence of bugs and developer confusion with reference counting, so I'm not at all keen to make it easier for the sake of potential use cases we can't articulate :/

[David Benjamin]

On Wed, Nov 18, 2015 at 2:47 PM Ryan Sleevi <rsl...@chromium.org> wrote:

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

In practice, this tends to work out extremely poorly.

A classic example it's let's imagine

class Foo {

 public:

  Foo(Bar* bar, Baz* baz);

};

This tells you some things about lifetime management, but not everything.

The immediate read should be that "Bar and Baz should remain valid for at least as long as the constructor, but likely as long as the object"

If that was perhaps implicitly guaranteed in some way - for example, if Baz was having ownership transferred, your reviewer should push on you to make that clearer:

Foo(Bar* bar, scoped_ptr<Baz> baz);

 

But we at least know that absent other information, Bar MUST remain valid for the duration of Foo. There's a tight coupling of those lifetimes.

Now, regardless of whether you're using RefCounted or shared_ptr<>, the only way to safely use 'foo' is to ensure that everything that may take a reference to Foo outlives Bar. That's hard. Hard for people and hard for machines to guarantee