You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jacob Barrett <jb...@pivotal.io> on 2017/09/18 22:23:18 UTC
Return types form methods on Cache
So, now that we have selected to use the value model for
CacheFactory::create()/Cache, we need to look at what the methods on Cache
return.
Starting with Cache::createRegionFactory method which returns
std::shared_ptr<RegionFactory>. I think it is very clear that this should
be a value type.
RegionFactory Cache::createRegionFactory(...) const;
Looking then at Cache::getRegion things are more interesting. It currently
returns std::shared_ptr<Region>. Internally we keep a set of these
shared_ptrs in a map. Does it make sense to keep returning these
shared_ptrs or should we again switch to a value model? It would be great
for consistency on the public API to go the value model way even if
initially the value simply holds a shared_ptr to the internal region impl.
Thoughts?
Re: Return types form methods on Cache
Posted by David Kimura <dk...@pivotal.io>.
Cool. +1 value model.
Thanks,
David
On Tue, Sep 19, 2017 at 11:15 AM, Jacob Barrett <jb...@pivotal.io> wrote:
> It isn’t. A Region should not outlive cache.
>
> > On Sep 19, 2017, at 10:54 AM, David Kimura <dk...@pivotal.io> wrote:
> >
> > Oh, man. I hope I didn't just kill the original idea. Unless we are
> > somehow using shared_from_this, it shouldn't be a divergence from
> existing
> > behavior?
> >
> > Thanks,
> > David
> >
> >> On Tue, Sep 19, 2017 at 10:47 AM, Jacob Barrett <jb...@pivotal.io>
> wrote:
> >>
> >> Region returned by this would no longer be valid. It’s “references” to
> >> interns cache/region would be invalid. The behavior is undefined.
> >>
> >>> On Sep 19, 2017, at 10:24 AM, David Kimura <dk...@pivotal.io> wrote:
> >>>
> >>> Err... I missed a return in example above.
> >>>
> >>> Region r = [](){ return CacheFactory::create().getRegion("myregion");
> }
> >> ();
> >>>
> >>>> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura <dk...@pivotal.io>
> >> wrote:
> >>>>
> >>>> I favor values, but we should probably be diligent.
> >>>>
> >>>> Do any of the objects returned from Cache depend on Cache to out-live
> >> the
> >>>> returned object? A quick scan suggested no, but we still have a
> >>>> std::enable_shared_from_this<Cache>. Maybe dead code. In code
> >> example,
> >>>> if this happens (for any cache.get*), could we be in trouble or is
> this
> >>>> user error?
> >>>>
> >>>> Region r = [](){ CacheFactory::create().getRegion("myregion"); }();
> >>>> // Cache is out of scope.
> >>>> // What is expected behavior?
> >>>> r.put("key", "value");
> >>>>
> >>>> Thanks,
> >>>> David
> >>>>
> >>>> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett <jb...@pivotal.io>
> >>>> wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>>> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
> >>>>>>
> >>>>>> I would vote +1 for a more attractive, professional and
> user-friendly
> >>>>> API.
> >>>>>> I'm not sure if there's a perf or memory-usage reason for using
> >>>>>> "std::shared_ptr<?>" to types instead of returning values, but the
> end
> >>>>>> result does not look like a professional API to me.
> >>>>>
> >>>>> There really isn’t, especially if you look at what we did dirty
> >>>>> CacheFactory::getCache by returning a value that can be moved into
> the
> >> heap
> >>>>> and a shared point of one wants but not being forced into it. RVO
> >> tricks
> >>>>> can event make that move a noop.
> >>>>>
> >>>>> auto r = cache.getRegion(...);
> >>>>> Where decltype(r) == Region
> >>>>> and
> >>>>> auto rp = std::make_shared<Region>(cache->getRegion());
> >>>>> Where decltype(rp) == shared_ptr<Region>
> >>>>>
> >>>>> Would both be valid.
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>
>
Re: Return types form methods on Cache
Posted by Jacob Barrett <jb...@pivotal.io>.
It isn’t. A Region should not outlive cache.
> On Sep 19, 2017, at 10:54 AM, David Kimura <dk...@pivotal.io> wrote:
>
> Oh, man. I hope I didn't just kill the original idea. Unless we are
> somehow using shared_from_this, it shouldn't be a divergence from existing
> behavior?
>
> Thanks,
> David
>
>> On Tue, Sep 19, 2017 at 10:47 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>>
>> Region returned by this would no longer be valid. It’s “references” to
>> interns cache/region would be invalid. The behavior is undefined.
>>
>>> On Sep 19, 2017, at 10:24 AM, David Kimura <dk...@pivotal.io> wrote:
>>>
>>> Err... I missed a return in example above.
>>>
>>> Region r = [](){ return CacheFactory::create().getRegion("myregion"); }
>> ();
>>>
>>>> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura <dk...@pivotal.io>
>> wrote:
>>>>
>>>> I favor values, but we should probably be diligent.
>>>>
>>>> Do any of the objects returned from Cache depend on Cache to out-live
>> the
>>>> returned object? A quick scan suggested no, but we still have a
>>>> std::enable_shared_from_this<Cache>. Maybe dead code. In code
>> example,
>>>> if this happens (for any cache.get*), could we be in trouble or is this
>>>> user error?
>>>>
>>>> Region r = [](){ CacheFactory::create().getRegion("myregion"); }();
>>>> // Cache is out of scope.
>>>> // What is expected behavior?
>>>> r.put("key", "value");
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett <jb...@pivotal.io>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>>> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
>>>>>>
>>>>>> I would vote +1 for a more attractive, professional and user-friendly
>>>>> API.
>>>>>> I'm not sure if there's a perf or memory-usage reason for using
>>>>>> "std::shared_ptr<?>" to types instead of returning values, but the end
>>>>>> result does not look like a professional API to me.
>>>>>
>>>>> There really isn’t, especially if you look at what we did dirty
>>>>> CacheFactory::getCache by returning a value that can be moved into the
>> heap
>>>>> and a shared point of one wants but not being forced into it. RVO
>> tricks
>>>>> can event make that move a noop.
>>>>>
>>>>> auto r = cache.getRegion(...);
>>>>> Where decltype(r) == Region
>>>>> and
>>>>> auto rp = std::make_shared<Region>(cache->getRegion());
>>>>> Where decltype(rp) == shared_ptr<Region>
>>>>>
>>>>> Would both be valid.
>>>>>
>>>>>
>>>>>
>>>>
>>
Re: Return types form methods on Cache
Posted by David Kimura <dk...@pivotal.io>.
Oh, man. I hope I didn't just kill the original idea. Unless we are
somehow using shared_from_this, it shouldn't be a divergence from existing
behavior?
Thanks,
David
On Tue, Sep 19, 2017 at 10:47 AM, Jacob Barrett <jb...@pivotal.io> wrote:
> Region returned by this would no longer be valid. It’s “references” to
> interns cache/region would be invalid. The behavior is undefined.
>
> > On Sep 19, 2017, at 10:24 AM, David Kimura <dk...@pivotal.io> wrote:
> >
> > Err... I missed a return in example above.
> >
> > Region r = [](){ return CacheFactory::create().getRegion("myregion"); }
> ();
> >
> >> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura <dk...@pivotal.io>
> wrote:
> >>
> >> I favor values, but we should probably be diligent.
> >>
> >> Do any of the objects returned from Cache depend on Cache to out-live
> the
> >> returned object? A quick scan suggested no, but we still have a
> >> std::enable_shared_from_this<Cache>. Maybe dead code. In code
> example,
> >> if this happens (for any cache.get*), could we be in trouble or is this
> >> user error?
> >>
> >> Region r = [](){ CacheFactory::create().getRegion("myregion"); }();
> >> // Cache is out of scope.
> >> // What is expected behavior?
> >> r.put("key", "value");
> >>
> >> Thanks,
> >> David
> >>
> >> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett <jb...@pivotal.io>
> >> wrote:
> >>
> >>>
> >>>
> >>>> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
> >>>>
> >>>> I would vote +1 for a more attractive, professional and user-friendly
> >>> API.
> >>>> I'm not sure if there's a perf or memory-usage reason for using
> >>>> "std::shared_ptr<?>" to types instead of returning values, but the end
> >>>> result does not look like a professional API to me.
> >>>
> >>> There really isn’t, especially if you look at what we did dirty
> >>> CacheFactory::getCache by returning a value that can be moved into the
> heap
> >>> and a shared point of one wants but not being forced into it. RVO
> tricks
> >>> can event make that move a noop.
> >>>
> >>> auto r = cache.getRegion(...);
> >>> Where decltype(r) == Region
> >>> and
> >>> auto rp = std::make_shared<Region>(cache->getRegion());
> >>> Where decltype(rp) == shared_ptr<Region>
> >>>
> >>> Would both be valid.
> >>>
> >>>
> >>>
> >>
>
Re: Return types form methods on Cache
Posted by Jacob Barrett <jb...@pivotal.io>.
Region returned by this would no longer be valid. It’s “references” to interns cache/region would be invalid. The behavior is undefined.
> On Sep 19, 2017, at 10:24 AM, David Kimura <dk...@pivotal.io> wrote:
>
> Err... I missed a return in example above.
>
> Region r = [](){ return CacheFactory::create().getRegion("myregion"); } ();
>
>> On Tue, Sep 19, 2017 at 10:19 AM, David Kimura <dk...@pivotal.io> wrote:
>>
>> I favor values, but we should probably be diligent.
>>
>> Do any of the objects returned from Cache depend on Cache to out-live the
>> returned object? A quick scan suggested no, but we still have a
>> std::enable_shared_from_this<Cache>. Maybe dead code. In code example,
>> if this happens (for any cache.get*), could we be in trouble or is this
>> user error?
>>
>> Region r = [](){ CacheFactory::create().getRegion("myregion"); }();
>> // Cache is out of scope.
>> // What is expected behavior?
>> r.put("key", "value");
>>
>> Thanks,
>> David
>>
>> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett <jb...@pivotal.io>
>> wrote:
>>
>>>
>>>
>>>> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
>>>>
>>>> I would vote +1 for a more attractive, professional and user-friendly
>>> API.
>>>> I'm not sure if there's a perf or memory-usage reason for using
>>>> "std::shared_ptr<?>" to types instead of returning values, but the end
>>>> result does not look like a professional API to me.
>>>
>>> There really isn’t, especially if you look at what we did dirty
>>> CacheFactory::getCache by returning a value that can be moved into the heap
>>> and a shared point of one wants but not being forced into it. RVO tricks
>>> can event make that move a noop.
>>>
>>> auto r = cache.getRegion(...);
>>> Where decltype(r) == Region
>>> and
>>> auto rp = std::make_shared<Region>(cache->getRegion());
>>> Where decltype(rp) == shared_ptr<Region>
>>>
>>> Would both be valid.
>>>
>>>
>>>
>>
Re: Return types form methods on Cache
Posted by David Kimura <dk...@pivotal.io>.
Err... I missed a return in example above.
Region r = [](){ return CacheFactory::create().getRegion("myregion"); } ();
On Tue, Sep 19, 2017 at 10:19 AM, David Kimura <dk...@pivotal.io> wrote:
> I favor values, but we should probably be diligent.
>
> Do any of the objects returned from Cache depend on Cache to out-live the
> returned object? A quick scan suggested no, but we still have a
> std::enable_shared_from_this<Cache>. Maybe dead code. In code example,
> if this happens (for any cache.get*), could we be in trouble or is this
> user error?
>
> Region r = [](){ CacheFactory::create().getRegion("myregion"); }();
> // Cache is out of scope.
> // What is expected behavior?
> r.put("key", "value");
>
> Thanks,
> David
>
> On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett <jb...@pivotal.io>
> wrote:
>
>>
>>
>> > On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
>> >
>> > I would vote +1 for a more attractive, professional and user-friendly
>> API.
>> > I'm not sure if there's a perf or memory-usage reason for using
>> > "std::shared_ptr<?>" to types instead of returning values, but the end
>> > result does not look like a professional API to me.
>>
>> There really isn’t, especially if you look at what we did dirty
>> CacheFactory::getCache by returning a value that can be moved into the heap
>> and a shared point of one wants but not being forced into it. RVO tricks
>> can event make that move a noop.
>>
>> auto r = cache.getRegion(...);
>> Where decltype(r) == Region
>> and
>> auto rp = std::make_shared<Region>(cache->getRegion());
>> Where decltype(rp) == shared_ptr<Region>
>>
>> Would both be valid.
>>
>>
>>
>
Re: Return types form methods on Cache
Posted by David Kimura <dk...@pivotal.io>.
I favor values, but we should probably be diligent.
Do any of the objects returned from Cache depend on Cache to out-live the
returned object? A quick scan suggested no, but we still have a
std::enable_shared_from_this<Cache>. Maybe dead code. In code example, if
this happens (for any cache.get*), could we be in trouble or is this user
error?
Region r = [](){ CacheFactory::create().getRegion("myregion"); }();
// Cache is out of scope.
// What is expected behavior?
r.put("key", "value");
Thanks,
David
On Mon, Sep 18, 2017 at 7:44 PM, Jacob Barrett <jb...@pivotal.io> wrote:
>
>
> > On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
> >
> > I would vote +1 for a more attractive, professional and user-friendly
> API.
> > I'm not sure if there's a perf or memory-usage reason for using
> > "std::shared_ptr<?>" to types instead of returning values, but the end
> > result does not look like a professional API to me.
>
> There really isn’t, especially if you look at what we did dirty
> CacheFactory::getCache by returning a value that can be moved into the heap
> and a shared point of one wants but not being forced into it. RVO tricks
> can event make that move a noop.
>
> auto r = cache.getRegion(...);
> Where decltype(r) == Region
> and
> auto rp = std::make_shared<Region>(cache->getRegion());
> Where decltype(rp) == shared_ptr<Region>
>
> Would both be valid.
>
>
>
Re: Return types form methods on Cache
Posted by Jacob Barrett <jb...@pivotal.io>.
> On Sep 18, 2017, at 7:34 PM, Kirk Lund <kl...@apache.org> wrote:
>
> I would vote +1 for a more attractive, professional and user-friendly API.
> I'm not sure if there's a perf or memory-usage reason for using
> "std::shared_ptr<?>" to types instead of returning values, but the end
> result does not look like a professional API to me.
There really isn’t, especially if you look at what we did dirty CacheFactory::getCache by returning a value that can be moved into the heap and a shared point of one wants but not being forced into it. RVO tricks can event make that move a noop.
auto r = cache.getRegion(...);
Where decltype(r) == Region
and
auto rp = std::make_shared<Region>(cache->getRegion());
Where decltype(rp) == shared_ptr<Region>
Would both be valid.
Re: Return types form methods on Cache
Posted by Kirk Lund <kl...@apache.org>.
I would vote +1 for a more attractive, professional and user-friendly API.
I'm not sure if there's a perf or memory-usage reason for using
"std::shared_ptr<?>" to types instead of returning values, but the end
result does not look like a professional API to me.
I would favor a more user-friendly API. If there's a performance or memory
constraint to returning value instead of shared ptr then applying such a
pattern to getRegion is overkill at the wrong level of abstraction -- the
user can easily keep the return value in a var instead of repeating the
Cache::getRegion call.
On Mon, Sep 18, 2017 at 3:23 PM, Jacob Barrett <jb...@pivotal.io> wrote:
> So, now that we have selected to use the value model for
> CacheFactory::create()/Cache, we need to look at what the methods on Cache
> return.
>
> Starting with Cache::createRegionFactory method which returns
> std::shared_ptr<RegionFactory>. I think it is very clear that this should
> be a value type.
>
> RegionFactory Cache::createRegionFactory(...) const;
>
>
> Looking then at Cache::getRegion things are more interesting. It currently
> returns std::shared_ptr<Region>. Internally we keep a set of these
> shared_ptrs in a map. Does it make sense to keep returning these
> shared_ptrs or should we again switch to a value model? It would be great
> for consistency on the public API to go the value model way even if
> initially the value simply holds a shared_ptr to the internal region impl.
>
> Thoughts?
>