Ok, one more idea. https://gist.github.com/pivotal-jbarrett/beed5f70c1f3a238cef94832b13dab7a
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 <jbarr...@pivotal.io> wrote: > Y'all here is an attempt to get the best of both worlds. > https://gist.github.com/pivotal-jbarrett/52ba9ec5de0b494368d1c5282ef188ef > > 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 <mmart...@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 <mdo...@pivotal.io >> > >> wrote: >> >> > +0 shared pointer >> > >> > > On 14 Sep, 2017, at 14:09, Ernest Burghardt <eburgha...@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 < >> > mdo...@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 <mhan...@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 <jbarr...@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 <dkim...@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 >> > >>>>> >> > >>> >> > >> >> > >> >> > >> > >> >
