Mailing-List: contact dev-help@geode.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geode.apache.org
MIME-Version: 1.0
In-Reply-To: <CAD=mLVqb1b64T=Uyan=nnG364YxNumnY_+N=BB2YiYKvzGxDgQ@mail.gmail.com>
References: <CAD=mLVraFBP4KPhaDGTagXmE8e5bLB9swoyvqmz3xdH8QGfk-w@mail.gmail.com>
 <CAA1wVxpzDkWQ0ML=d4a9jUvfJnFw6fSL=m2ykRGi1aWhepi5CA@mail.gmail.com>
 <07F3EC88-79D0-4B2D-A467-B74DC5DA5D63@pivotal.io> <F244A43D-80AE-4B95-84B8-5E844CA2B4ED@pivotal.io>
 <CAB6Kt2542P8n-18LB7cEoCv-Tn0CK0RKcqqp_iFib_1Vynxxmw@mail.gmail.com>
 <46427510-AF4A-45DC-93DA-D79F1B31610B@pivotal.io> <CAD5Ss1TvfLMLU_3PSOKNb=riR3+N2vYcZ5HChH+B-zCF4o=Wjg@mail.gmail.com>
 <CAA1wVxrJ1c_Vwy3aJ2s4YUtbKv2eyinsO-G3o2NV5+Sb=JLyVQ@mail.gmail.com>
 <CAA1wVxpTAnhm2vAH0txd-QvKFqTOU4LgUte4qQdv43Q9D4PvHw@mail.gmail.com> <CAD=mLVqb1b64T=Uyan=nnG364YxNumnY_+N=BB2YiYKvzGxDgQ@mail.gmail.com>
From: Ernest Burghardt <eburghardt@pivotal.io>
Date: Mon, 18 Sep 2017 13:24:33 -0600
Message-ID: <CAB6Kt25YM7yqw_KCX8Yt1m6V9yrzhuu4zhjUUvARA8mnXBAL-Q@mail.gmail.com>
Subject: Re: [Discuss] Investigation of C++ object return types
To: dev@geode.apache.org
Content-Type: multipart/alternative; boundary="001a1143d48056a1a905597bb1ba"
archived-at: Mon, 18 Sep 2017 19:24:44 -0000

--001a1143d48056a1a905597bb1ba
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

+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 a=
ny
> 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 o=
f
> it
> > things look much better. They aren't perfect since we still need the ba=
ck
> > pointer from the impl to the cache for later use. If we didn't need tha=
t
> > 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 clarit=
y
> I
> > added an XmlCacheConfig object to that builds up the CacheConfig via Xm=
l.
> > 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 poin=
t
> > > since we only maintain a unique_ptr. After moving into a shared_ptr i=
t
> > 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'v=
e
> > >> evolved to just use C# and Java :)
> > >>
> > >> Without investing a lot more time, I don't have strong feelings abou=
t
> > raw
> > >> vs shared pointers. My only question is: Can we return ptr to abstra=
ct
> > >> 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 lifetim=
e
> > >> 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 =E2=80=9Cpointer" approach that is identified in t=
he 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 publ=
ic
> > >> 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 =3D dataOutput.getCache();
> > >> > >>>> ...
> > >> > >>>> }
> > >> > >>>> In this the value of cache will RVO the allocation of Cache i=
n
> > the
> > >> > >> getCache
> > >> > >>>> call into the stack of this method, great. The problem is tha=
t
> > >> 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 =3D 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 valu=
e:
> > >> > >>>>>
> > >> > >>>>>  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 =3D CacheFactory::createFactory().create();
> > >> > >>>>>
> > >> > >>>>>  // Here user creates smart pointer in their heap.
> > >> > >>>>>  auto cptr =3D
> > >> > >>>>> 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 tha=
t
> > >> 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 allocatio=
ns
> > >> from
> > >> > >>>>> factory using shared pointers.
> > >> > >>>>>
> > >> > >>>>> Thanks,
> > >> > >>>>> David
> > >> > >>>>>
> > >> > >>>
> > >> > >>
> > >> > >>
> > >> >
> > >> >
> > >>
> > >
> >
>

--001a1143d48056a1a905597bb1ba--