11

I have an shared object that need to be send to a system API and extract it back later. The system API receives void * only. I cannot use shared_ptr::get() because it do not increases the reference count and it could be released by other threads before extract from the system API. Sending a newed shared_ptr * will work but involves additional heap allocation.

One way to do it is to let the object derived from enable_shared_from_this. However, because this class template owns only a weak_ptr, it is not enough to keep the object from released.

So my solution looks like the following:

class MyClass:public enable_shared_from_this<MyClass> {
private:
    shared_ptr<MyClass> m_this;
public:
    void *lock(){
        m_this=shared_from_this();
        return this;
    }
    static shared_ptr<MyClass> unlock(void *p){
        auto pthis = static_cast<MyClass *>(p);
        return move(pthis->m_this);
    }
/* ... */
}

/* ... */
autp pobj = make_shared<MyObject>(...);
/* ... */
system_api_send_obj(pobj->lock());
/* ... */
auto punlocked = MyClass::unlock(system_api_reveive_obj());

Are there any easier way to do this?

The downside of this solution:

  • it requires an additional shared_ptr<MyClass> in the MyClass object layout, in addition of a weak_ptr in the base class enable_shared_from_this.

  • As I mentioned in the comments, access to lock() and unlock() concurrently is NOT Safe.

  • The worst thing is that this solution can only support lock() once before a call of unlock(). If the same object is to be use for multiple system API calls, additional reference counting must be implemented.

If we have another enable_lockable_shared_from_this class it will be greate:

class MyClass:public enable_lockable_shared_from_this<MyClass> {
/* ... */
}

/* ... */
autp pobj = make_shared<MyObject>(...);
/* ... */
system_api_send_obj(pobj.lock());
/* ... */
auto punlocked = unlock_shared<MyClass>(system_api_reveive_obj());

And the implementation of enable_lockable_shared_from_this is similar as enable_shared_from_this, the only difference is it implements lock() and a helper function unlock_shared. The calling of those functions only explicitly increase and decrease use_count(). This will be the ideal solution because:

  • It eliminate the additional space cost

  • It reuses the facilities existing for shared_ptr to guarantee concurrency safety.

  • The best thing of this solution is that it supports multiple lock() calls seamlessly.

However, the only biggest downside is: it is not available at the moment!

UPDATE:

At least two answers to this question involves a container of pointers. Please compare those solutions with the following:

class MyClass:public enable_shared_from_this<MyClass> {
private:
    shared_ptr<MyClass> m_this;
    mutex this_lock; //not necessory for single threading environment
    int lock_count;
public:
    void *lock(){
        lock_guard lck(this_lock); //not necessory for single threading environment
        if(!lock_count==0)
            m_this=shared_from_this();
        return this;
    }
    static shared_ptr<MyClass> unlock(void *p){
        lock_guard lck(this_lock); //not necessory for single threading environment
        auto pthis = static_cast<MyClass *>(p);
        if(--lock_count>0)
            return pthis->m_this;
        else {
            lock_count=0;
            return move(pthis->m_this); //returns nullptr if not previously locked
        }
    }
/* ... */
}

/* ... */
autp pobj = make_shared<MyObject>(...);
/* ... */
system_api_send_obj(pobj->lock());
/* ... */
auto punlocked = MyClass::unlock(system_api_reveive_obj());

This is absolutely an O(1) vs O(n) (space; time is O(log(n)) or similar, but absolutely greater than O(1) ) game.

Earth Engine
  • 10,048
  • 5
  • 48
  • 78
  • 1
    "Easier way"? Does that solution even work? What good is a `MyClass` pointer to the API function? Basically, whoever controls the `system_api_set_key()` and `system_api_get_key()` should keep a shared pointer around, so that the lifetime of the object is suitably managed. – Kerrek SB Nov 09 '11 at 23:19
  • This is working because the object own a strong reference to itself when locked, and this is a cyclic reference until m_this is released. system_api_send_key (sorry I changed the name) and system_api_receive_key is SYSTEM API and I cannot change them. – Earth Engine Nov 09 '11 at 23:25
  • The only thing I need to mention is that accessing m_this by multiple thread at the same time is NOT safe. shared_ptr guarantee the multithreading access to the the reference object is safe, but not the shared_ptr itself. So we need some locking surrounding the `lock` and `unlock` functions calls. – Earth Engine Nov 09 '11 at 23:31
  • Hmm... but your `lock()` returns a `MyClass*` -- what sort of API understands that? Also, I'm not sure if moving from a shared pointer does what you want -- wouldn't `reset()` or so be the better thing to do? – Kerrek SB Nov 09 '11 at 23:33
  • 1
    Some API enables you to pass a void * as a "handle" that only understandable by the client; the system just store it somewhere and return it in the future. Yes, the code is the same as `shared_ptr tmp=m_this; m_this.reset(); return tmp;` in C++11. the move constructor guarantee the above. – Earth Engine Nov 09 '11 at 23:38
  • Alright then. So is this question really about concurrency, or about using `shared_ptr` to manage object lifetime? – Kerrek SB Nov 09 '11 at 23:44
  • This question is about object lifetime, not concurrency. Concurrency is not necessary related. – Earth Engine Nov 09 '11 at 23:46
  • In that case it all depends on who's calling `system_api_send_obj()`... could you give a complete, minimal desired usage example that representatively shows your requirements and difficulties? – Kerrek SB Nov 09 '11 at 23:48
  • A simple example is the `PostMessage()` and `GetMessage()` WIN32 API. You can MSDN it. – Earth Engine Nov 09 '11 at 23:50
  • From what I can see, the messages passed through PostMessage and GetMessage are integers. Are you planning to pass a pointer as an integer? – Vaughn Cato Nov 10 '11 at 03:37
  • 1
    @Vaughn : The messages passed through PostMessage and GetMessage are integers specifically sized to be the same size as pointers. I.e., planning to pass a pointer as an integer is perfectly okay in that scenario. – ildjarn Nov 10 '11 at 19:12
  • @Earth : If you used [Boost.SmartPointers](http://www.boost.org/libs/smart_ptr/)' [`boost::intrusive_ptr<>`](http://www.boost.org/libs/smart_ptr/intrusive_ptr.html) instead of `shared_ptr<>`, then `MyClass` could contain its own ref-count and you could manually increment/decrement it as you see fit. – ildjarn Nov 11 '11 at 00:24
  • I think you mean pinning and not locking. – Mahmoud Al-Qudsi Nov 18 '11 at 07:34
  • @MahmoudAl-Qudsi em... You could be right, the word "locking" sometimes confusing. In this article, "locking" have nothing to do with concurrency, it just means keeping the object alive. – Earth Engine Nov 20 '11 at 22:28

4 Answers4

3

I am now have an idea of the following:

template<typename T>
struct locker_helper{
    typedef shared_ptr<T> p_t;
    typedef typename aligned_storage<sizeof(p_t), alignment_of<p_t>::value>::type s_t;
};
template<typename T> void lock_shared(const shared_ptr<T> &p){
    typename locker_helper<T>::s_t value;
    new (&value)shared_ptr<T>(p);
}
template<typename T> void unlock_shared(const shared_ptr<T> &p){
    typename locker_helper<T>::s_t value
        = *reinterpret_cast<const typename locker_helper<T>::s_t *const>(&p);
    reinterpret_cast<shared_ptr<T> *>(&value)->~shared_ptr<T>();
}


template<typename T>
void print_use_count(string s, const shared_ptr<T> &p){
    cout<<s<<p.use_count()<<endl;
}

int main(int argc, char **argv){
    auto pi = make_shared<int>(10);
    auto s = "pi use_count()=";
    print_use_count(s, pi); //pi use_count()=1
    lock_shared(pi);
    print_use_count(s, pi);//pi use_count()=2
    unlock_shared(pi);
    print_use_count(s, pi);//pi use_count()=1
}

and then we can implement the original example as following:

class MyClass:public enable_shared_from_this { /*...*/ };

/* ... */
auto pobj = make_shared<MyClass>(...);
/* ... */
lock_shared(pobj);
system_api_send_obj(pobj.get());
/* ... */
auto preceived = 
    static_cast<MyClass *>(system_api_reveive_obj())->shared_from_this();
unlock_shared(preceived);

It is easy to implement a enable_lockable_shared_from_this with this idea. However, the above is more generic, allows control things that is not derived from enable_lockable_from_this` template class.

Earth Engine
  • 10,048
  • 5
  • 48
  • 78
  • This was along the lines I was thinking. I see a couple problems though, which is why I didn't post it: that `reinterpret_cast` in `unlock_shared` is, AFAIK, either implementation-defined or undefined. If the latter, solution is a no-go to me, if the former, not preferable in the least, but if you have no other options document it well and use it. The other problem is that if your API never does the callback, because you've artificially increased the reference count, it'll never be free'd. – GManNickG Nov 10 '11 at 10:01
  • you are using stack overflow wrong. only post answers, or edit your question, but do not post answers that are question. I.e., stack overflow does not use the classy forum style posting, the OP always is the one question, all other posts are answers. – Sebastian Mach Nov 10 '11 at 10:06
  • I agree there is an issue that `reinterpret_cast` is not portable. This problem can only be fixed by the shared_ptr author to provide something like `enable_lockable_shared_from_this`. The API problem is not an issue, since we allways assume the API is correct, otherwise we will submit an error report. – Earth Engine Nov 10 '11 at 10:12
  • @phresnel I have already removed the question and now it is an answer. – Earth Engine Nov 10 '11 at 10:16
  • @Earth Engine: That's of course a perfectly legit way of using SO :) – Sebastian Mach Nov 10 '11 at 10:17
  • 1
    The problem I see is that in `unlock_shared` you are doing a copy of the memory of `shared_ptr` just like if you did a `memcpy` and then you are calling the destructor in that copy. But this is UB since the `shared_ptr` class is not trivally copyable. – rodrigo Nov 10 '11 at 15:48
  • Good point. `shared_ptr` can never be trivially copyable because it need to track the reference count. However, this tracking of reference count is exactly what I want to bypass. – Earth Engine Nov 10 '11 at 22:48
  • @EarthEngine Bit still UB. Anyway, I've checked the G++ implementation, and the memory layout of a shared_ptr is just two pointers: one to the object itself and one to the counter. And since the destructor touch neither, you could implement `unlock_shared` with just one line: `p.~shared_ptr();` – rodrigo Nov 11 '11 at 09:41
  • @rodrigo If you look deep into a specific implementation, you will always be able to found optimized solutions. However, to prevent any future adjustments to the implementation breaks your program, you have to assume you do not know how they implement it. And actually my solution is not 100% implementation independent; if the shared_ptr author decide to use "relative" pointers, we are still in trouble. However this not likely to be the case for any benefits. On the other hand, clearing the pointers after destruction could be a way for diagnostic, so it could not be ignored. – Earth Engine Nov 20 '11 at 22:31
  • @EarthEngine You say that your solution is 'implementation dependent', but it is actually 'undefined behavior'. And these are two very different things. My former suggestion had a bit of irony in it: it is actually no better or worse than yours, it is just simpler and thus more obviously wrong. – rodrigo Nov 21 '11 at 00:00
  • @rodrigo Let me explain. I would like to expect that nobody implement `shared_ptr` with relative pointers, but some day somebody else (of another team in the company) could possibly implement a debug version of `shared_ptr` that clears the pointers at destruction. So this is the different. – Earth Engine Nov 21 '11 at 00:30
  • @EarthEngine I understand that. But you should note that you are relying both in implementation details of the library, and also in a particular behavior that is explicitly specified as _undefined_ by the language. All of that to gain a probably negligible performance gain. Personally, I find it unwise. – rodrigo Nov 21 '11 at 11:23
  • @rodrigo you can refer to my update. I listed an 100% well defined solution and is better than container solutions. The only cost is an additional pointer, an additional counter, and an optional mutex for multithreading. My opinion is that locking should be implemented by `shared_ptr` itself. Since both std and boost does not implemented it yet, my `RawCopy` solution is a workaround. If you care about portability, you should use the "counted locking" solution instead. – Earth Engine Nov 21 '11 at 21:34
  • I had the same idea. It's a shame that there is no public interface to increase/decrease the reference counter, that would make it so much cleaner and easier. (A while back, we had an own shared_ptr implementation in our project with functions like these public and in some cases this was quite useful.) – Albert Nov 21 '11 at 22:03
3

By adjusting the previous answer, I finally get the following solution:

//A wrapper class allowing you to control the object lifetime
//explicitly.
//
template<typename T> class life_manager{
public:
    //Prevent polymorphic types for object slicing issue.
    //To use with polymorphic class, you need to create
    //a container type for storage, and then use that type.
    static_assert(!std::is_polymorphic<T>::value, 
        "Use on polymorphic types is prohibited.");

    //Example for support of single variable constructor
    //Can be extented to support any number of parameters
    //by using varidict template.
    template<typename V> static void ReConstruct(const T &p, V &&v){ 
        new (const_cast<T *>(&p))T(std::forward<V>(v));
    }

    static void RawCopy(T &target, const T &source){
        *internal_cast(&target) = *internal_cast(&source);
    }
private:
    //The standard said that reterinterpret_cast<T *>(p) is the same as 
    //static_cast<T *>(static_cast<void *>(p)) only when T has standard layout.
    //
    //Unfortunately shared_ptr do not.
    static struct { char _unnamed[sizeof(T)]; } *internal_cast(const T *p){
        typedef decltype(internal_cast(nullptr)) raw;
        return static_cast<raw>(static_cast<void *>(const_cast<T *>(p)));
    }
};

//Construct a new instance of shared_ptr will increase the reference
//count. The original pointer was overridden, so its destructor will
//not run, which keeps the increased reference count after the call.
template<typename T> void lock_shared(const std::shared_ptr<T> &p){
    life_manager<shared_ptr<T> >::ReConstruct(p, std::shared_ptr<T>(p));
}

//RawCopy do bit-by-bit copying, bypassing the copy constructor
//so the reference count will not increase. This function copies
//the shared_ptr to a temporary, and so it will be destructed and
//have the reference count decreased after the call.
template<typename T> void unlock_shared(const std::shared_ptr<T> &p){
    life_manager<shared_ptr<T> >::RawCopy(std::shared_ptr<T>(), p);
}

This is actually the same as my previous version, however. The only thing I did is to create a more generic solution to control object lifetime explicitly.

According to the standard(5.2.9.13), the static_cast sequence is definitely well defined. Furthermore, the "raw" copy behavior could be undefined but we are explicitly requesting to do so, so the user MUST check the system compatibility before using this facility.

Furthermore, actually only the RawCopy() are required in this example. The introduce of ReConstruct is just for general purpose.

Earth Engine
  • 10,048
  • 5
  • 48
  • 78
2

Why not just wrap the void * API in something that tracks the lifetime of that API's references to your object?

eg.

typedef std::shared_ptr<MyClass> MyPtr;
class APIWrapper
{
    // could have multiple references to the same thing?
    // use multiset instead!
    // are references local to transient messages processed in FIFO order?
    // use a queue!  ... etc. etc.
    std::set<MyPtr, SuitableComparator> references_;

public:
    void send(MyPtr const &ref)
    {
        references_.insert(ref);
        system_api_send_obj(ref.get());
    }
    MyPtr receive(void *api)
    {
        MyPtr ref( static_cast<MyClass *>(api)->shared_from_this() );
        references_.erase(ref);
        return ref;
    }
};

Obviously (hopefully) you know the actual ownership semantics of your API, so the above is just a broad guess.

Useless
  • 64,155
  • 6
  • 88
  • 132
  • I also thought about this solution; however the std::set also involves in additional costs, includes time and space. – Earth Engine Nov 10 '11 at 03:29
  • If I were you, I will prefer to have a std::map, and it reduces the call of shared_from_this. Furthermore, you still have to protect the references_ from concurrently access. – Earth Engine Nov 10 '11 at 03:32
  • Firstly, there is nowhere near enough information in the question to determine whether the time & space overhead you mention is an issue - do you have a constraint you didn't mention? In fact, using a map instead of a set to save a function call is another time/space tradeoff, and it all smells of premature optimisation unless you're omitting something relevant. Secondly, you explicitly said the question wasn't about concurrency - is there anything else you'd like to add? – Useless Nov 10 '11 at 03:44
  • While it does involve "time and space", this is likely the easiest solution when working with APIs that don't allow the passing of smart pointers. – Chad Nov 21 '11 at 21:46
1

How about using the following global functions that use pointers-to-smart-pointers.

template<typename T> void *shared_lock(std::shared_ptr<T> &sp)
{
    return new std::shared_ptr<T>(sp);
}
template<typename T> std::shared_ptr<T> shared_unlock(void *vp)
{
    std::shared_ptr<T> *psp = static_cast<std::shared_ptr<T,D>*>(sp);
    std::shared_ptr<T> res(*psp);
    delete psp;
    return res;
}

You have an extra new/delete but the original type is unmodified. And additionally, concurrency is not an issue, because each call to shared_lock will return a different shared_ptr.

To avoid the new/delete calls you could use an object pool, but I think that it is not worth the effort.

UPDATE:

If you are not going to use multiple threads in this matter, what about the following?

template<typename T> struct SharedLocker
{
    std::vector< std::shared_ptr<T> > m_ptrs;

    unsigned lock(std::shared_ptr<T> &sp)
    {
        for (unsigned i = 0; i < m_ptrs.size(); ++i)
        {
            if (!m_ptrs[i])
            {
                 m_ptrs[i] = sp;
                 return i;
            }
        }
        m_ptrs.push_back(sp);
        return m_ptrs.size() - 1;
    }
    std::shared_ptr<T> unlock(unsigned i)
    {
        return std::move(m_ptrs[i]);
    }
};

I've changed the void* into an unsigned, but that shouldn't be a problem. You could also use intptr_t, if needed.

Assuming that most of the time there will be only a handful of objects in the vector, maybe even no more than 1, the memory allocations will only happen once, on first insertion. And the rest of the time it will be at zero cost.

rodrigo
  • 94,151
  • 12
  • 143
  • 190
  • 1
    `std::shared_ptr *psp = static_cast*>(sp);` could be rewritten as `std::unique_ptr> psp(static_cast*>(sp));`, and then an explicit `delete` wouldn't be necessary (i.e., you could cut `shared_unlock`'s implementation from 4 lines to 2). :-] – ildjarn Nov 10 '11 at 01:38
  • 2
    There's no such thing as `shared_ptr`. `shared_ptr` is famously templated **only** on the type; deleter and allocator are type-erased implementation details. – Kerrek SB Nov 10 '11 at 01:38
  • This is what I said "Sending a newed shared_ptr * will work but involves additional heap allocation. " – Earth Engine Nov 10 '11 at 02:03
  • I will still prefer enable_lockable_shared_from_this; it cuts almost all costs. – Earth Engine Nov 10 '11 at 02:06
  • @EarthEngine Yeah, I didn't see your comment about the heap allocation on first read. But anyway, if you plan to send the `void*` through a system call, the extra new/delete should be the least of your worries. "Be wary of premature optimization". Anyway you can look at the updated answer. – rodrigo Nov 10 '11 at 09:21