geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hanson <mhan...@pivotal.io>
Subject Re: [Discuss] Investigation of C++ object return types
Date Mon, 18 Sep 2017 18:43:23 GMT
Sounds good to me. +1

> On Sep 18, 2017, at 11:36 AM, David Kimura <dkimura@pivotal.io> wrote:
> 
> 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 <mhanson@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 <dkimura@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 <mhanson@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 <dkimura@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 <jbarrett@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 <jbarrett@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 <mmartell@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 <
>>>>>> mdodge@pivotal.io
>>>>>>>>> 
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> +0 shared pointer
>>>>>>>>> 
>>>>>>>>>> On 14 Sep, 2017, at 14:09, Ernest Burghardt <
>> eburghardt@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 <
>>>>>>>>> mdodge@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 <mhanson@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
<
>> jbarrett@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 <
>> dkimura@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
>>>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>> 
>>>> 
>> 
>> 


Mime
View raw message