[bind10-dev] Shared pointers, references and guideline

[JINMEI Tatuya / 神明達哉]

At Wed, 29 May 2013 14:26:16 +0200,
Michal 'vorner' Vaner <michal.vaner at nic.cz> wrote:

> I thought there was a guideline to pass shared pointers as:
> 
>   void function(const shared_ptr<Data>& data)
> 
> instead
> 
>   void function(shared_ptr<Data> data)
> 
> if possible. The reason is, of course, performance, because we measured we spend
> several percent of run time creating and destroying shared pointers.

I remember the discussion but I don't remember agreed consensus for
the guideline.

Although I'm not necessarily opposed to this style strongly, it looks
awkward if not counter intuitive or confusing, making me ponder the
intent (increasing the time to understand the code while reading it).
When I see a const reference parameter such as "const Foo& foo", I'd
consider foo is immutable and the caller maintains its ownership.
While it's still the case whether Foo is a (smart) pointer type or
not (of course), but at least to me it can be confusing and I'd need
to use some brain cycle to understand it's not the object foo points
to but foo (pointer) itself that is immutable.  Also, at the risk of
disclosing my limited experience of browsing other code, I think I've
not seen this style of passing shared pointers elsewhere.  If it's
really a less common style, and there's not other reason for doing
something uncommon, I'd generally follow commonly used styles not to
surprise other people.

As for performance, although you probably actually didn't mean
creating/destroying but the copying overhead of shared pointers (I
suspect create/destroy overhead isn't substantial unless it's
gaining the first reference or releasing the last one), my general
understanding is that this overhead looked large partly because the
overall performance of b10-auth was much poorer than today.  Now that
it should be significantly faster compared to that version, my guess
is that it's now much more marginal.  At least I'd run the benchmark
to measure the copy overhead for the latest version of b10-auth before
making any decision on this for the reason of performance.

I'd also note that there should be several cases where the passed
pointer isn't expected to be empty (NULL) and the pointer isn't
actually expected to be shared with the caller.  If the copy overhead
really still matters, I'd first consider identifying these cases and
replacing them with passing a reference to the pointed object.

If, after doing these things, the copy overhead of shared pointers is
still deemed to be non marginal, I wouldn't object to adopting this
practice.

Another issue on this point is consistency.  I realize we're currently
mixing the two styles: passing bare shared pointers and passing const
references to them.  In general, it's better to use a consistent
style throughout the source tree, so I wouldn't be oppose to switching
to the reference style even if the performance benefit is marginal if:

- others in the project have no problem with this style in terms of
  understandability, and
- this style is actually not so uncommon (or even a commonly adopted
  style) in general

---
JINMEI, Tatuya
Internet Systems Consortium, Inc.