1

This can be seen as a follow up to e.g. Why shared pointer assignment does 'swap'?.

The question is about the Copy&Swap idiom used e.g. in boost.

I do understand that the benefit of Copy&Swap is to reuse existing code which avoids duplication and bugs. But there are 2 cases (actually 1 can be reduced to the other) where it is not optimal:

  1. The smart pointer instances are the same
  2. The contained pointer is the same

For shared_ptr the ref counters are incremented atomically and for intrusive_ptr(boost only) they may be. So there is a high cost for a copy.

This can be avoided, if the assignment was implemented like:

smart_ptr& operator=(const smart_ptr& other){
  if(this->ptr_ == other.ptr_) return *this;
  smart_ptr(other).swap(*this); // I assume I can simply do this here, right?
  return *this;
}
smart_ptr& operator=(smart_ptr&& other){
  smart_ptr(std::move(other)).swap(*this);
  return *this;
}

Wouldn't this be the fastest and safest implementation or is there any issue I did not see?

If it is the fastest, why aren't boost or the stdlib using it?

To clarify on point 2. consider the following code:

smart_ptr a(new foo);
auto b = a;
...
// Eventually:
a = b;

This is not self-assignment as &a != &b. The Copy&Swap does involve an unnecessary modification of the reference counter.

Flamefire
  • 5,313
  • 3
  • 35
  • 70
  • copy ctor isn't called, it is already optimized as they are swap, move and copy pointers only. – The Unknown Jul 30 '18 at 11:15
  • The self assignment is a very uncommon case. With copy and swap you get rid of the self assignment test and gain a little bit of extra performance for the common case and yeah on top its good code reuse. – phön Jul 30 '18 at 11:28
  • it is not about the pointees copy ctor and the smart_ptr copy ctor **is** called. @phön I added a snipped to clarify that it is not (only) self-assignment in which case performance is lost – Flamefire Jul 30 '18 at 11:34
  • branching is more costly then useless small copy operation. This is result of how CPU is optimized. So it is better not to perform checking. Maybe someone will provide a link about this (I don't have time to look for it)? – Marek R Jul 30 '18 at 11:36
  • @Flamefire Well i think even this case is very uncommon. I am not sure if this is the only argument for the copy and swap besides code reuse vs the naive implementation – phön Jul 30 '18 at 11:41

1 Answers1

0

The self assignment is not a common case so it will cost more to test that each time then to swap it either way. Also copying a pointer is basically the fastest copy one can perform.

The real cost of copying shared_ptr is atomic reference increment (which may involve using mutex underneath).

If you really want to test performance of both approach I would recommend getting google benchmark library and write a set of test cases (for self assigment and all other cases) and to measure it. Remember that optimizer these days can do marvels with your code to optimize it. Without measuring it, it would be hard to tell if this is faster, but I guess as if are pretty expensive the version without it is better :)

EDIT:

If you don't want to reference count to increase (which is expensive part with copying shared_ptr) you can always use move constructor:

smart_ptr a(new foo);
auto b = a;
...
// Eventually:
a = std::move(b);
wdudzik
  • 1,264
  • 15
  • 24
  • This misses the point, that a copy of `smart_ptr` involves an expensive atomic increment/decrement of the ref counter. It is **not** just copying the pointer. I explicitly used `shared_ptr`/`intrusive_ptr` as an example. For `unique_ptr` you'd be correct. – Flamefire Jul 30 '18 at 11:41
  • Sorry, I assumed that edit made it more clear. I added few words about it :) hope that it can help someone in future. – wdudzik Jul 30 '18 at 11:46
  • @wdudzik Well in your code snippet the code has different behaviour and is probably not what the OP wanted, since it "dodges" the problem OP asked about – phön Jul 30 '18 at 11:56
  • Correct. The `b` might be stored in a class, moved around, come from somewhere else (registry maybe?) and would be accessible e.g. by a get member function of some class. So moving it from there is not an option. – Flamefire Jul 30 '18 at 12:41