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 <[email protected]> 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
>
