+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 <dkim...@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 <jbarr...@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 <jbarr...@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 <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 > > >> > >>>>> > > >> > >>> > > >> > >> > > >> > >> > > >> > > > >> > > > >> > > > > > >
