Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ownership management policies #1

Closed
marton78 opened this issue May 12, 2012 · 24 comments
Closed

Ownership management policies #1

marton78 opened this issue May 12, 2012 · 24 comments

Comments

@marton78
Copy link

Hi Vinnie,

I think it makes sense to continue the discussion here.

Your suggestion to implement policies was the following:

classname can be expanded to include a policy. Given:

struct Policy
{
  virtual void push () = 0;
  virtual void addref () = 0;
  virtual void release () = 0;

  template <class PolicyType>
  static Policy* getPolicy ()
  {
    static PolicyType policy;
    return &policy;
  }
};

We can add this:

template <typename T>
struct classname
{
  static inline Policy& policy()
  {
     return *classname <T>::m_policy;
  }
  template <class PolicyType>
  static inline void set_policy ()
  {
    classname <T>::m_policy = &Policy::getPolicy <PolicyType> ();
  }

private:
  static PolicyType* m_policy;
};

template <typename T>
PolicyType* classname <T>::m_policy = Policy::getPolicy <DefaultPolicy> ();

Then we expand class__ to include an additional PolicyType template parameter, and set the policy in the classname at the same time we associate the name with it:

template <typename T, class PolicyType>
class__<T>::class__ (lua_State *L_, const char *name_): scope(L_, name_)
{
  assert(!classname<T>::is_const());
  classname<T>::set_name(name_);
  classname<T>::set_policy <typename PolicyType> ();

Here are some examples of code using this scheme:

s.class_ <test3> ("test3").method ("f", &test3::f); // default policy
s.class_ <test3, BoostSharedPtrPolicy> ("test3").method ("f", &test3::f); // use boost::shared_ptr
s.class_ <test3, CustomPolicy> ("test3").method ("f", &test3::f); // use domain specific policy
s.class_ <test3, ImmortalPolicy> ("test3").method ("f", &test3::f); // straight pointer with no ref count policy

Comments, criticisms?

@marton78
Copy link
Author

I like your suggiestion with policies! However, I'm not sure I understand why this complicated construction with the abstract base class and classname is necessary. Couldn't it be done the following way?

template <typename T, typename Enable = void>
struct default_policy
{
    typedef T base_type;
    typedef shared_ptr<T> storage_type;

    static base_type dereference(void* p)
    {
        return *((storage_type*)p)->get();
    }

    static void store(void* mem, T data)
    {
        new(mem) storage_type(new T(data));
    }

    // probably more stuff

};

template <typename T>
struct default_policy<T, enable_if<is_smart_pointer<T>::value>::type>
{
    typedef T::element_type base_type;
    typedef T storage_type;

    static base_type dereference(void* p)
    {
        return *((T*)p)->get();
    }

    static void store(void* mem, T data)
    {
        new(mem) storage_type(data);
    }
};

//etc, other specializations for T*, T&, const...

template <typename T>
struct value_semantics_policy
{
    typedef T base_type;
    typedef T storage_type;

    static base_type dereference(void* p)
    {
        return *((T*)p);
    }

    static void store(void* mem, T data)
    {
        new(mem) storage_type(data);
    }
};

// value_semantics_policy doesn't need more specializations

template <typename T, typename Policy = default_policy<T> >
struct tdstack
{
    typedef typename Policy::base_type base_type;
    typedef typename Policy::storage_type storage_type;

    static void push (lua_State *L, base_type data)
    {
        // Make sure we don't try to push objects of
        // unregistered classes or primitive types
        assert(classname<base_type>::name() != classname_unknown);

        // Allocate a new userdata and construct the pointer in-place there
        void *block = lua_newuserdata(L, sizeof(storage_type));
        store(block, std::move(data));

        // Set the userdata's metatable
        luaL_getmetatable(L, classname<base_type>::name());
        lua_setmetatable(L, -2);
    }

    static T get (lua_State *L, int index)
    {
        // Make sure we don't try to retrieve objects of
        // unregistered classes or primitive types
        assert(classname<base_type>::name() != classname_unknown);

        return dereference(checkclass(L, index, classname<base_type>::name());
    }
};

template <typename T, class Policy = default_policy<T> >
class__;
// uses tdstack<Policy>

Personally I'd use value semantics: If I want a shared_ptr, I'll specify that when pushing/getting my data:
shared_ptr<MyClass> p = get_it_from_luabridge_forgot_the_syntax_now<shared_ptr<MyClass> >(L);

@vinniefalco
Copy link
Owner

You're right that coming from the tdstack / C++ side of things, the compile time template solution is completely workable. But what if the Lua code creates the object via the constructor binding? There needs to be a system where we can specify the style of container to use for certain cases, at the time of class registration. Then later, use that system. This is why I have the weird looking static "policy" pointer, so that we can make the decision at run time and use it.

I got some of these ideas working, and they did look different from my original example. When I got to the tdstack part of things is where it broke down. I have engineered a new solution which looks much better than the previous one and I am putting it into the demo app.

@vinniefalco
Copy link
Owner

tdstack is definitely where a lot of action takes place. After some experimentation I believe that it is not enough to specify one policy for a class. I think that it is necessary for the same class to have multiple instantiations, each with their own style of lifetime management. There are three categories of lifetime management:

  • Fully managed by C++: Pointers and references fall in this category.
  • Fully managed by Lua: Constructing the object directly in the userdata. Pass by value falls into this category.
  • Managed by a container: A pointer to the object is created dynamically, and the container is constructed inside the userdata.

I abstracted the storage of classes in userdata and called it "class Userdata" which you can see in this experimental implementation:
https://github.com/vinniefalco/LuaBridgeDemo/blob/master/LuaBridge/LuaBridge.h

There are three subclasses of Userdata, to handle the three use-cases described above:

UserdataByValue (fully managed by Lua)
UserdataByReference (fully managed by C++)
UserdataByContainer (managed by container)

A pointer to the class can be obtained through the base Userdata class, so it is always possible for C++ to get a raw pointer. With this system, it is possible to support references as lvalues, and all the other missing features in tdstack (where push() is private). This overcomes some of the original design limitations of the library - C++ can receive pointers to objects which do not need to be wrapped at all in a shared_ptr. C++ can also receive writable references to objects.

With this ability of course comes the risk of misuse. Someone could write a function that receives a pointer to an object fully managed by Lua, where they remember that pointer value and try to use it after its been garbage collected. Users of the library will just need to be careful, and understand what they are doing.

This opens up powerful use cases. For example, the C++ can pass a Rectangle structure by value into Lua. Then, Lua can call a bound C++ function which takes a writable reference to the Rectangle. The C++ function can modify the struct, and the Lua will get it back. All this happens with the object constructed directly inside the userdata (via placement new). I have this working right now in the demo project.

It should also possible to take the same Rectangle object, and push it on the stack wrapped in a shared_ptr (or any other template parameterized container). This can be detected with a tdstack specialization, and then the UserdataByContainer subclass gets created instead. The result is to capability to have the same Rectangle class type, exist with different styles of lifetime management, determined by the function signatures.

@vinniefalco
Copy link
Owner

To answer your original question regarding the pointer to the abstract policy base class in the classname traits template, it seems to be the only way I could think of to control the style of storage for the constructor() binding. For the case of Lua creating class objects, we can rule out UserdataByReference (fully managed by C++). That leaves us only with by value, or by container.

So there needs to be a way to specify at compile time, for all objects of a particular class created by Lua (via the constructor() registration), whether to have by value or by container style. If the choice is by container style, then we need to be able to specify at compile time the type of container.

None of this should prevent objects of that class from being created on the C++ side and used with different management styles. This is easier to do by modifying the tdstack as you have shown in your examples.

@vinniefalco
Copy link
Owner

These are some tdstack snippets that show the usage of the Userdata subclasses:

/**
  Lua stack type-dispatch for objects with value semantics

  @note Pointers and references are specialized separately.
*/
template <typename T>
struct tdstack
{
public:
  /**
    Push a copy of a registered class onto the stack.

    @note T must be copy-constructible.
  */
  static void push (lua_State* L, T t)
  {
    UserdataByValue <T>::push (L, t);
  }

  /**
    Retrieve a copy of a registered class from the stack.

    @note T must be copy-constructible.
  */
  static T get (lua_State* L, int index)
  {
    return UserdataByValue <T>::get (L, index);
  }
};

Here's one that works for any shared_ptr wrapped object (like before):

template <typename T>
struct tdstack <shared_ptr<T> >
{
  static void push (lua_State* L, shared_ptr<T> p)
  {
    UserdataByContainer <T, shared_ptr>::push (L, *p);
  }

  static shared_ptr<T> get (lua_State* L, int index)
  {
    return UserdataByContainer <T, shared_ptr>::get (L, index);
  }
};

And here's one that works for lvalue references:

template <typename T>
struct tdstack <T&>
{
  static void push (lua_State* L, T& t)
  {
    UserdataByReference <T>::push (L, t);
  }

  static T& get (lua_State* L, int index)
  {
    return *Userdata::getClassPointer <T> (L, index);
  }
};

Note that push() above was private in the original luabridge. Also note that get() goes through Userdata::getClassPointer(). This means we can obtain a writable reference to ANY class regardless of its policy - both of these uses are of course subject to the risks mentioned previously.

@marton78
Copy link
Author

Hmm. Thinking about it a second time, the way LuaBridge handles the storage of objects internally should be completely independent of a class's syntax. Which means, class__ should be oblivious to any such details, policies should only be of interest when requesting storage/retrieval of a particular object. Which means, a userdata could be anything: T or T* or some_ptr<T>, you name it.

There is already the mechanism of checking if a retrieved class is the same type as the stored class: the checkclass function, which checks if the metatable of the requested object is the same as the metatable with a given name. I guess it would make sense to consider an alternative mechanism (maybe additionally): in tdstack::push, allocate a block of size sizeof(T)+sizeof(const std::type_info*) and store &typeid(T) alongside with the object. This way, a simple pointer equality check would suffice in tdstack::get.

@vinniefalco
Copy link
Owner

Yes, that sounds exactly right. And you listed the three use cases: T, T*, and some_ptr<T>. I can't think of anything beyond those three. class__ should not need to know anything about policies. However, the constructor() registration function is the place where we need to provide for a storage policy - the choices are T (UserdataByValue<T>) or some_ptr<T> (UserdataByContainer <T, some_ptr>).

The way I have designed the Userdata class, keeping a pointer to the std::type_info* as a data member would be trivial. To be clear, are you suggesting this as an optimization? Or would this provide some additional functionality? checkClass() does do a fair amount of heavy lifting.

But it also handles the case where the object in question is a subclass. This means that a B* can be passed where A* or some_ptr<A> is expected. I'm not sure how this would be accomplished with std::type_info, although I'm willing to try.

@vinniefalco
Copy link
Owner

Now I understand why you asked this question...
http://stackoverflow.com/questions/10539305/generic-way-to-test-if-a-type-is-a-smart-pointer

I'm having the same problem now.

@marton78
Copy link
Author

You're right, when calling the constructor from Lua, the policy must be specified beforehand. I hadn't thought of that. Then I suppose it's only reasonable to default to creating a shared_ptr<T> (but with make_shared!) in the constructor_proxy and give the possibility to customize that behaviour when wrapping the constructor. When pushing an object from C, all options should be selectable at run-time.

The const std::type_info<T>* (must be const) stored alongside the value is necessary to check if the stored object is T, T* or some_shared_ptr_type<T>. For example, if a raw T is stored (i.e., the object is owned by Lua), and a shared_ptr is requested as return type, it must be constructed with a null deleter.

Btw, the three cases directly indicate ownership: T: Lua, T*: C++, shared_ptr<T>: well, shared :)

@vinniefalco
Copy link
Owner

Marton: First I'd like to thank you for taking the time to work with me - your input has been incredibly valuable. I really want to make LuaBridge the most popular C++ bindings library.

Second, I have committed a preliminary implementation of almost everything we talked about. With these changes, all of the restrictions in the original luabridge implementation regarding naked pointers and references have been lifted (with the usual dangers of lifetime management).

C++ can get a pointer or reference to any registered class no matter what the lifetime policy. The resulting pointer is readable, and writable (if non const). I generalized the tdstack specialization for shared_ptr-like containers:

template <class T, template <class> class SharedPtr>
struct tdstack <SharedPtr <T> >
{
  static void push (lua_State* L, SharedPtr <T> p)
  {
    UserdataBySharedPtr <T, SharedPtr>::push (L, *p);
  }

  static SharedPtr <T> get (lua_State* L, int index)
  {
    return UserdataBySharedPtr <T, SharedPtr>::get (L, index);
  }
};

You should be able to freely use any container that has the same interface as shared_ptr. It is not necessary to "register" the container beforehand, or expose it through traits. It should be deduced. Conversions up a derivation hierarchy should also work; SharedPtr<B> can be passed where SharedPtr<A> is expected, if B is a subclass of A. Conversions between different shared pointer containers should also work SharedPtr2 <A> can be passed where SharedPtr1 <A> is expected if there is a conversion path although I have not tested this (I could use help writing the unit tests).

No matter what container you use (if you use any at all) it is always possible to receive a class by value, by reference, or by pointer, with preserved const qualities. I'm not sure about volatile (I could use help with that).

What is missing is the customization of the object lifetime policy of the registered constructor() function. Currently, objects created in Lua by calling a registered constructor will be stored as luabridge::shared_ptr just like before.

I put in as much robustness for supporting const objects as I could but I ran into a little roadblock, I will open a separate issue for it now.

There is also another bug I discovered, I will open an issue for that too. Your comments or criticisms on these issues, and my code changes are of course welcomed.

@vinniefalco
Copy link
Owner

Marton: you said "The const std::type_info* (must be const) stored alongside the value is necessary to check if the stored object is T, T* or some_shared_ptr_type". I believe that I have implemented an alternative to that, through the subclasses.

UserdataByValue is the case of T, UserdataByReference is T*, and UserdataBySharedPtr is for some_shared_ptr_type . The decision on which subclass to instantiate is made by tdstack on a per-specialization basis.

@vinniefalco
Copy link
Owner

Marton: You said "When pushing an object from C, all options should be selectable at run-time." I think this is fully implemented, if you would like to have a look at tdstack<>::push() variations.

@marton78
Copy link
Author

Vinnie, I don't have enough time right now to check your changes, but I'm reading the docs you've written. Some notes:

You write:

  • T, T const : Pass T by value. The lifetime is managed by Lua.
  • T*, T&,T const*,T const&: PassT` by reference. The lifetime is managed by C++.

I think it's ambiguous to support passing objects by reference, since the user cannot specify that when invoking a function. Therefore, passing T and T& should have the same behaviour. Only direct (i.e., self-documenting) qualification should change ownership semantics, i.e. x, &x and some_ptr<x> or boost:: / std::ref<x> (c++11)

Any container may be used. LuaBridge expects that the container in question is a class template with one template argument, and a member function called get() which returns a pointer to the underlying object.

boost:: and std::shared_ptr has a second, optional Allocator template parameter.

@vinniefalco
Copy link
Owner

I understand what you're saying, but this is not the case for invocation of functions from C++, these ownership semantics are determined by the function signature at the time of registration. Consider

void foo (T t);
scope s(L).method ("foo", &foo);

When Lua passes a T to foo it is clear from the function signature that pass by value is used. The lifetime management of the instance of T from which t was copied doesn't matter (since t is a copy). Compare with this registration:

void foo (T& t);
scope s(L).method ("foo", &foo);

Again, it is unambiguous that t is a reference. This will work no matter what the lifetime management of the particular t. In fact, the same foo could be called several times in a row, each with a T that uses a different lifetime management policy.

Since its essentially a pointer (references and pointers are dealt with the same way internally), you could get into trouble if you remember the pointer between calls. So don't do that.

The case where object passing is ambiguous is on the C++ side, when placing an existing object into the Lua stack. Should the object be passed by reference? Or by value? Fortunately, all usages of tdstack follow your "self-documenting qualification" requirement since they take a template argument:

T t;
tdstack <T>::push (L, t); // by value
tdstack <T&>::push (L, t); // by reference (potential danger)

shared_ptr <T> p (new T);
tdstack <T>::push (L, *p.get()); // by value
tdstack <shared_ptr <T> >::push (L, p); // by shared_ptr

tdstack <T*>::push (L, p.get()); // by reference (potential danger)

The potential danger occurs when Lua keeps the userdata around but the C++ destroys the object (in the example above, t could go out of scope before Lua performs the garbage collection). To avoid these situations, just don't push references or take reference arguments in bound functions (but the feature is there for you when it makes sense to use it).

I'm going to probably add a simple and convenient push template function that lets you skip the repetition of the template parameters. At that point, it will be necessary to have a self-documenting qualification. This was my thinking:

template <class T>
inline void pushValue (lua_State* L, T t)
{
  tdstack <T>::push (L, t);
}

template <class T>
inline void pushRef (lua_State* L, T t)
{
  tdstack <T&>::push (L, t);
}

template <class T>
inline void pushRef (lua_State* L, T* t)
{
  tdstack <T*>::push (L, t);
}

You're right that std::shared_ptr takes a second optional parameter. And I want LuaBridge to support those containers transparently. Unfortunately, my template skills still need work. Initially I tried passing the container type more generally (class SharedPtr instead of template <class> class SharedPtr), and having a provision for the policy clone / rebind<> idiom (http://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Policy_Clone).

I couldn't get that working right so I decided to fall back on the rigid template implementation and get everything else working first. After its all correct and settles down then I can take another crack at generalizing the container type.

@vinniefalco
Copy link
Owner

Any thoughts on this?

@vinniefalco
Copy link
Owner

Marton: I just got finished with a big rewrite of the library. I resolved all of the issues with the container (it is a generic template parameter now). I've expanded the tests to include this use case. All of the pointer and reference conversions work as well. I fixed several bugs. I rewrote all of the documentation. If you would be so kind as to review the 'develop' branch and provide your feedback, it would be greatly appreciated.

@vinniefalco
Copy link
Owner

This issue has been fully resolved in the develop branch. Simply add a specialization for ContainerTraits in the luabridge namespace for your custom container:

  template <class T>
  struct ContainerTraits <CustomContainer <T> >
  {
    typedef typename T Type;

    static T* get (CustomContainer <T> const& c)
    {
      return c.getPointerToObject ();
    }
  };

LuaBridge will automatically handle the rest.

@marton78
Copy link
Author

Nice. I am reading your code now. In my modifications to luabridge (internal unfortunately) I have had some of the same ideas as you did. For example, to key metatables using process-wide unique ids, only that mine are generated via

template <typename T>
inline void* class_id()
{
    struct x {};
    return (void*) typeid(x).hash_code();
};

But hash_code needs C++11. Your solution might be easier on memory, since typeid() generates a string internally, however, I don't know if it's not optimized away. Also, to get the ID of a const T, I simply use class_id(const T).

Also me I put the const methods additionally into the non-const metatable to save one lookup.

If I'm not mistaken, you also implemented the change I did too, to leave a class and its metatables on stack when registering its methods, to avoid looking it up every time.

Additionally to your changes, I got rid of all closures when pushing the proxies and to calculate the class uid inside the proxies on the fly. It might be faster (haven't benchmarked it though) and for sure it simplifies code.

@vinniefalco
Copy link
Owner

Yes it seems that we independently came up with the same improvements. I prefer not to require C++11, although it certainly would have made some things easier. I also got rid of some of the upvalues, although it is not entirely clear how you can get rid of all of them (for example, methodProxy needs the pointer to member). Note that lua_pushcfunction is simply a macro for lua_pushcclosure with 0 upvalues.

@marton78
Copy link
Author

You can get rid of the pointer to member by specifying it at compile time:

template <typename T, typename FnPtr, FnPtr F>
struct method_proxy;

template <typename T, typename Ret, Ret (T::*F)( /* params */ )>
struct method_proxy<T, Ret (T::*) ( /* params */ ), F>
{
    static int f(lua_State* L)
    {
        // ...
        T* obj = /*  the first param on stack */;
        Ret r = (obj->*F)( /* the params */);
        // ...
    }
};

Should be faster, too.

And then:

#define method(name, ptr) method__<decltype(ptr), (ptr)>(name)        

template <typename T>
template <typename FnPtr, FnPtr fp>
inline Class<T>& Class<T>::method__(const char* method_name)
{
    typedef method_proxy<T, FnPtr, fp> proxy;

    lua_pushcfunction(L, &proxy::f);

    // add const member functions only to const table
    if (std::is_const<typename proxy::class_type>::value)
    {
        lua_pushvalue(L, -1);
        add_overload(L, index + 1, method_name);
    }

    // add all member functions to non-const table
    add_overload(L, index + 2, method_name);

    return *this;
}

@marton78
Copy link
Author

PS: But you are right, one upvalue is still needed for free function pointers. But for methods and properties, you can do without them, as described above. Note: the decltype command needs C++11 but you could substitute it by stealing code from BOOST_TYPEOF

@vinniefalco
Copy link
Owner

Very nice. Compact, but dense. Is it possible to eliminate the method macro by putting another template in between method__ and proxy::f that has the <decltype (ptr), (ptr)> bit?

@vinniefalco
Copy link
Owner

Marton: Can I just add a typedef ... Type to each existing specialization of FuncTraits in order to implement decltype? Then I could write typename FuncTraits <ptr>::Type as a replacement for decltype(ptr).

@vinniefalco
Copy link
Owner

Moved this to a new issue:

#10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants