Application code doesn't need to invoke move (and probably shouldn't). In the examples, I think it's more of an exercise to show what would happen if invoked. And really, I expect same badness to happen using shared pointer method if we dereferenced a moved pointer.
Thanks, David On Mon, Sep 18, 2017 at 11:19 AM, Mark Hanson <mhan...@pivotal.io> wrote: > If you think this is a significant ease of use benefit, why have to invoke > move? I understand this code, but I have not seen it in recent memory. > I agree that this may make use of a copy(move) constructor easier, but... > > If the consensus is that this is a significant ease of use benefit for the > user, I am on board… > > +1 > > Thanks, > Mark > > On Sep 18, 2017, at 11:15 AM, David Kimura <dkim...@pivotal.io> wrote: > > > > 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 > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >> > >> > >
