geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Kimura <dkim...@pivotal.io>
Subject Re: [Discuss] Investigation of C++ object return types
Date Mon, 18 Sep 2017 18:36:35 GMT
Application code doesn't need to invoke move (and probably shouldn't).  In
the examples, I think it's more of an exercise to show what would happen if
invoked.  And really, I expect same badness to happen using shared pointer
method if we dereferenced a moved pointer.

Thanks,
David

On Mon, Sep 18, 2017 at 11:19 AM, Mark Hanson <mhanson@pivotal.io> wrote:

> If you think this is a significant ease of use benefit, why have to invoke
> move? I understand this code, but I have not seen it in recent memory.
> I agree that this may make use of a copy(move) constructor easier, but...
>
> If the consensus is that this is a significant ease of use benefit for the
> user, I am on board…
>
> +1
>
> Thanks,
> Mark
> > On Sep 18, 2017, at 11:15 AM, David Kimura <dkimura@pivotal.io> wrote:
> >
> > The benefit I see isn't for performance, it's flexibility and ease of use
> > by application developer.  Anything we can do to achieve this, I think
> is a
> > significant win.
> >
> > I think the reason for the various approaches is to determine whether we
> > can safely create the simple/flexible API without incurring a penalty on
> > performance.  Personally, I think the no nullptr benefit that Sarge
> > mentioned in previous thread is enough to warrant this approach serious
> > thought even though it provides no performance benefit.
> >
> > Thanks,
> > David
> >
> > On Mon, Sep 18, 2017 at 10:47 AM, Mark Hanson <mhanson@pivotal.io>
> wrote:
> >
> >> Hi All,
> >>
> >> As we are creating a user API, unless there is a significant performance
> >> benefit, I think we should try to take the simpler route.
> >>
> >> I see benefit to the approach being proposed, but I don’t see a
> >> significant benefit. Someone please school me if I am wrong.
> >>
> >> I am still in the shared pointer camp for cache and a raw pointer to
> cache
> >> in cacheImpl.
> >>
> >> Sorry, and thanks,
> >> Mark
> >>
> >>
> >>
> >>> On Sep 18, 2017, at 10:14 AM, David Kimura <dkimura@pivotal.io> wrote:
> >>>
> >>> +1 value approach (via some implementation from this thread)
> >>>
> >>> I think I like this.
> >>>
> >>> In all BAD cases, it's the user who shot themselves in the foot by
> using
> >>> std::move unsafely.  I expect this behavior is the same behavior as for
> >> any
> >>> other object.  And if we're ever able to get rid of the Cache/CacheImpl
> >>> circular dependency then we can add a copy constructor.
> >>>
> >>> I also like the idea of passing in a CacheConfig.  My concern though is
> >>> that it's piling on another big change.
> >>>
> >>> Thanks,
> >>> David
> >>>
> >>> On Sun, Sep 17, 2017 at 12:02 AM, Jacob Barrett <jbarrett@pivotal.io>
> >> wrote:
> >>>
> >>>> Ok, one more idea.
> >>>> https://gist.github.com/pivotal-jbarrett/
> beed5f70c1f3a238cef94832b13dab
> >> 7a
> >>>>
> >>>> The biggest issue with the value model is that we have been using a
> >> factory
> >>>> to build the Cache object. We really don't need one and if we get rid
> >> of it
> >>>> things look much better. They aren't perfect since we still need the
> >> back
> >>>> pointer from the impl to the cache for later use. If we didn't need
> that
> >>>> then we could allow copy construction. As it stands right now this
> >> version
> >>>> allows value, shared_prt, unique_ptr, etc. without any real overhead
> or
> >> RVO
> >>>> issues.
> >>>>
> >>>> The big change is that, rather than have a factory that we set a bunch
> >> of
> >>>> values on and then ask it to create the Cache, we create a CacheConfig
> >>>> object and pass that in to the Cache's constructor. Cache passes it
to
> >>>> CacheImpl and CacheImpl sets itself up based on the config. If you
> look
> >> at
> >>>> what the current factory model does it isn't that different. For
> >> clarity I
> >>>> added an XmlCacheConfig object to that builds up the CacheConfig via
> >> Xml.
> >>>> You could imagine a YamlCacheConfig object *shiver*. The point is we
> >> don't
> >>>> care as long as we get a CacheConfig with all the things we support
at
> >>>> "init" time.
> >>>>
> >>>> I know it is a more radical change but I feel it is more C++ and more
> >>>> testable than the factory model. I also like that it solves some of
> the
> >>>> issues with the value model we were looking at.
> >>>>
> >>>> -Jake
> >>>>
> >>>> On Thu, Sep 14, 2017 at 5:16 PM Jacob Barrett <jbarrett@pivotal.io>
> >> wrote:
> >>>>
> >>>>> Y'all here is an attempt to get the best of both worlds.
> >>>>> https://gist.github.com/pivotal-jbarrett/
> >> 52ba9ec5de0b494368d1c5282ef188
> >>>> ef
> >>>>>
> >>>>> I thought I would try posting to Gist but so far not impressed,
> sorry.
> >>>>>
> >>>>> The Gist of it is that we can overcome the thirdpary or transient
> >>>>> reference back to the public Cache instance by keeping a reference
to
> >> it
> >>>> in
> >>>>> the implementation instance and updating it whenever the move
> >> constructor
> >>>>> is called.
> >>>>>
> >>>>> The downside is if your run this test it doesn't show RVO kicking
in
> on
> >>>>> the second test where we move the value into a shared pointer. There
> >> are
> >>>> a
> >>>>> couple of pitfalls you can stumble into as well by trying to used
the
> >>>>> previous instance to access the cache after the move operation,
as
> >>>>> illustrated by the "BAD" commented lines.
> >>>>>
> >>>>> The upside is the choices this gives the use for ownership of their
> >>>>> factory constructed Cache instance. They can keep it a value or
move
> it
> >>>> to
> >>>>> unique or shared pointer.
> >>>>>
> >>>>> Overhead wise I think we better off in value as long as there are
no
> >>>>> moves, rare I would thing, but the moves are pretty cheap at the
> point
> >>>>> since we only maintain a unique_ptr. After moving into a shared_ptr
> it
> >>>> acts
> >>>>> identical to the shared_ptr model proposed earlier.
> >>>>>
> >>>>> -Jake
> >>>>>
> >>>>>
> >>>>> On Thu, Sep 14, 2017 at 3:36 PM Michael Martell <mmartell@pivotal.io
> >
> >>>>> wrote:
> >>>>>
> >>>>>> Late to this party.
> >>>>>>
> >>>>>> Confession 1: I had to look up both RVO and copy-elision.
> >>>>>> Confession 2: I don't like using pointers at all. I used to,
but
> I've
> >>>>>> evolved to just use C# and Java :)
> >>>>>>
> >>>>>> Without investing a lot more time, I don't have strong feelings
> about
> >>>> raw
> >>>>>> vs shared pointers. My only question is: Can we return ptr to
> abstract
> >>>>>> class everywhere we return objects? Just thinking of mocking,
which
> >>>> always
> >>>>>> wants to mock interfaces.
> >>>>>>
> >>>>>> On Thu, Sep 14, 2017 at 2:25 PM, Michael William Dodge <
> >>>> mdodge@pivotal.io
> >>>>>>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> +0 shared pointer
> >>>>>>>
> >>>>>>>> On 14 Sep, 2017, at 14:09, Ernest Burghardt <
> eburghardt@pivotal.io>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> Calling a vote for:
> >>>>>>>>
> >>>>>>>> - Raw pointer
> >>>>>>>> - shard pointer
> >>>>>>>>
> >>>>>>>> +1 raw Pointer, I had to look up RVO and am new to std::move(s)
> >>>>>>>>
> >>>>>>>> On Thu, Sep 14, 2017 at 3:02 PM, Michael William Dodge
<
> >>>>>>> mdodge@pivotal.io>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> I generally dig reference-counted pointers for avoiding
lifetime
> >>>>>> issues
> >>>>>>>>> with objects allocated off the heap but I can live
with bare
> >>>>>> pointers,
> >>>>>>> too.
> >>>>>>>>>
> >>>>>>>>> Sarge
> >>>>>>>>>
> >>>>>>>>>> On 13 Sep, 2017, at 16:25, Mark Hanson <mhanson@pivotal.io>
> >>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi All,
> >>>>>>>>>>
> >>>>>>>>>> I favor the “pointer" approach that is identified
in the code
> >>>>>> sample.
> >>>>>>>>> There is greater clarity and less bytes seemingly
created and
> >>>>>> written.
> >>>>>>> We
> >>>>>>>>> do sacrifice the potential ease of using an object,
but in all, I
> >>>>>> think
> >>>>>>> the
> >>>>>>>>> way our code is structured. It is not conducive
to do a value
> >>>>>> approach,
> >>>>>>>>> from an efficiency standpoint,  because of our use
of the pimpl
> >>>>>> model.
> >>>>>>>>>>
> >>>>>>>>>> Thanks,
> >>>>>>>>>> Mark
> >>>>>>>>>>
> >>>>>>>>>>> On Sep 12, 2017, at 11:09 AM, Jacob Barrett
<
> jbarrett@pivotal.io
> >>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> My biggest concern with this model is that
access to the public
> >>>>>> Cache
> >>>>>>>>>>> object from other public objects results
in additional
> >>>> allocations
> >>>>>> of
> >>>>>>> a
> >>>>>>>>>>> Cache value. Think about when we are inside
a Serializable
> object
> >>>>>> and
> >>>>>>> we
> >>>>>>>>>>> access the Cache from DataOutput.
> >>>>>>>>>>>
> >>>>>>>>>>> As value:
> >>>>>>>>>>> Serializable* MyClass::fromData(DataInput&
dataInput) {
> >>>>>>>>>>> auto cache = dataOutput.getCache();
> >>>>>>>>>>> ...
> >>>>>>>>>>> }
> >>>>>>>>>>> In this the value of cache will RVO the
allocation of Cache in
> >>>> the
> >>>>>>>>> getCache
> >>>>>>>>>>> call into the stack of this method, great.
The problem is that
> >>>>>> Cache
> >>>>>>>>> must
> >>>>>>>>>>> contain a std::shared_ptr<CacheImpl>
which means that each
> >>>>>> allocation
> >>>>>>>>> is 8
> >>>>>>>>>>> bytes (pointer to control block and pointer
to CacheImpl) as
> well
> >>>>>> as
> >>>>>>>>> having
> >>>>>>>>>>> to increment the strong counter in the control
block. On
> >>>>>> exit/descope,
> >>>>>>>>> the
> >>>>>>>>>>> Cache will have to decrement the control
block as well.
> >>>>>>>>>>>
> >>>>>>>>>>> Using current shared_ptr pimple model:
> >>>>>>>>>>> Serializable* MyClass::fromData(DataInput&
dataInput) {
> >>>>>>>>>>> auto& cache = dataOutput.getCache();
> >>>>>>>>>>> ...
> >>>>>>>>>>> }
> >>>>>>>>>>> We only suffer the ref allocation of 4 bytes
and no ref count.
> >>>>>> Since
> >>>>>>>>> this
> >>>>>>>>>>> function call can't survive past the lifespan
of
> Cache/CacheImpl
> >>>>>> they
> >>>>>>>>> don't
> >>>>>>>>>>> need to have shared_ptr and refcounting.
> >>>>>>>>>>>
> >>>>>>>>>>> Given that this method could be called numerous
times is the
> >>>>>> overhead
> >>>>>>> of
> >>>>>>>>>>> the value version going to be a significant
performance issue?
> >>>>>>>>>>>
> >>>>>>>>>>> I worry that moves and RVO is just beyond
most developers.
> Heck I
> >>>>>>> didn't
> >>>>>>>>>>> know anything about it until we started
exploring it.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> -Jake
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Sep 12, 2017 at 8:06 AM David Kimura
<
> dkimura@pivotal.io
> >>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Follow up of attached discussion after
more investigation.  I
> >>>>>> created
> >>>>>>>>> an
> >>>>>>>>>>>> example of returning Cache as shared
pointer versus raw value:
> >>>>>>>>>>>>
> >>>>>>>>>>>> https://github.com/dgkimura/geode-native-sandbox
> >>>>>>>>>>>>
> >>>>>>>>>>>> I still like returning by value as it
lets the user do what
> they
> >>>>>> want
> >>>>>>>>> with
> >>>>>>>>>>>> their object.
> >>>>>>>>>>>>
> >>>>>>>>>>>> // Here user creates object on their
stack.
> >>>>>>>>>>>> auto c = CacheFactory::createFactory().create();
> >>>>>>>>>>>>
> >>>>>>>>>>>> // Here user creates smart pointer in
their heap.
> >>>>>>>>>>>> auto cptr =
> >>>>>>>>>>>> std::make_shared<Cache>(CacheFactory::createFactory().
> >>>> create());
> >>>>>>>>>>>>
> >>>>>>>>>>>> Difficulty of implementing this is high
due to circular
> >>>>>> dependencies
> >>>>>>> of
> >>>>>>>>>>>> Cache/CacheImpl as well as objects hanging
off CacheImpl that
> >>>>>> return
> >>>>>>>>>>>> Cache.  We must be extra careful when
dealing with move/copy
> >>>>>>> semantics
> >>>>>>>>> of
> >>>>>>>>>>>> Cache/CacheImpl.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Alternative, is to keep as is and only
permit heap allocations
> >>>>>> from
> >>>>>>>>>>>> factory using shared pointers.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks,
> >>>>>>>>>>>> David
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message