geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Barrett <jbarr...@pivotal.io>
Subject Re: [Discuss] Investigation of C++ object return types
Date Mon, 18 Sep 2017 20:46:38 GMT
Yup! It works.

https://gist.github.com/pivotal-jbarrett/c48ffff3f7f41b187f0ed8c80108aa6a

I also corrected the solution to use a unique_ptr for the Cache to
CacheImpl relationship.

Notice the main.cpp uses CacheFactory model as well as the direct
allocation model with CacheConfig. So if we ship with the current
CacheFactory model unchanged we can update later to support both where
CacheFactory will just create a CacheConfig and construct a new Cache the
same as you would directly.

Woot!

-Jake


On Mon, Sep 18, 2017 at 12:40 PM Jacob Barrett <jbarrett@pivotal.io> wrote:

> So I am changing my vote to Value as well. It was great to put together
> all these samples because it really helped me understand what it buys us.
>
> With the current factory model it’s a little less appealing but not
> significantly enough to really notice. I would say we go GA with the
> current factory model and then quickly deprecate it for the non factory
> model. I think we can have both side by side until the next major release
> without issue. I’ll whip up a sample to make sure.
>
> -Jake
>
>
> > On Sep 18, 2017, at 12:24 PM, Ernest Burghardt <eburghardt@pivotal.io>
> wrote:
> >
> > +1 to passing in CacheConfig  - might be a bit of refactoring (needed)
> but
> > complexity level should be low
> >
> > +1 value approach as well as dumping the Factory
> >
> >
> >
> >> On Mon, Sep 18, 2017 at 11: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