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 <mhan...@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 <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 > >>>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>> > >>> > >> > >
