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 >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >> >>
