Add share Info

[HGuillemet]

shared_ptr<C> are handled by specifying this info:

new Info("std::shared_ptr<C>")
  .annotations("@SharedPtr")
  .valueTypes("@Cast({\"\", \"std::shared_ptr<C>\"}) C")
  .pointerTypes("C")

which results in the ability to use Java class C indifferently to represent a native C or a shared_ptr<C>.
This works well, but this is incomplete, because if a c = new C() is instantiated, and then passed to a native function taking a shared_ptr<C>, and then the instance is deallocated, either explicitly with c.deallocate(), or by a PointerScope, or by the GC, the native object will be freed, causing a SIGSEGV if the native library tries to access c. Thus ruining the point of using shared pointers. Currently we can add this Info on C constructor :

new Info("C::C").annotations("@NoDeallocator")

to avoid the SIGSEGV, but then instances of C will never be freed. We need to be able to call make_shared from Java-side.

This PR adds a new Info:

new Info("C").share()

that:

1. does the same things than

new Info("std::shared_ptr<C>")
  .annotations("@SharedPtr")
  .valueTypes("@Cast({\"\", \"std::shared_ptr<C>\"}) C")
  .pointerTypes("C")

2. changes the constructor from the usual:

public C() { super((Pointer) null); allocate();
private native void allocate();

to:

public C() { super(makeShared()); }
@Namespace @SharedPtr @Name("SHARED_PTR_NAMESPACE::make_shared<C>") static private native C makeShared();

What do you think of this solution ?

An alternative, a bit more efficient, to makeShared, is to add a @SharedPtr annotation to allocate, and have the Generator output an alternate version of allocate, but that would need to patch Generator and its magic is a bit to dark for me.

[saudet]

An alternative, a bit more efficient, to makeShared, is to add a @SharedPtr annotation to allocate, and have the Generator output an alternate version of allocate, but that would need to patch Generator and its magic is a bit to dark for me.

Yes, we should be doing that. allocate() is just a normal JNI function, you can look as where @NoDeallocator gets used. That's probably where something like this should be. We might as well generalize this and check for @Adapter instead and wrap make_shared in a static function of SharedPtrAdapter.

And with the Parser, we could let users provide an Info.annotations(...) on the constructor I guess.

[HGuillemet]

I can try something in this direction.

Also I'm wondering about the relevance of generating the @SharedPtr, @Cast and @PointerTypes automatically in the parser from a Info.share(). In pytorch you wrote some variants: like here and or here.
What was the logic there and is that implementable in the Parse ?

[saudet]

I'm sure it could be done somehow, but those are just hacks because the C++ compiler isn't able to resolve ambiguities. We'd need adapters with more explicit forms of type conversions as per issue #374 (comment). /cc @equeim

[HGuillemet]

For handling const in SharedPtrAdapter, I think that we could instantiate the template only for non-const types, and add a constructor accepting a const with something like this:

template<class T> class SharedPtrAdapter {
public:
    typedef SHARED_PTR_NAMESPACE::shared_ptr<T> S;
    typedef SHARED_PTR_NAMESPACE::shared_ptr<const T> SC;
    SharedPtrAdapter(const T* ptr, size_t size, void* owner) : ptr((T*)ptr), size(size), owner(owner),
            sharedPtr2(owner != NULL && owner != ptr ? *(S*)owner : S((T*)ptr)), sharedPtr(sharedPtr2) { }
    SharedPtrAdapter(const S& sharedPtr) : ptr(0), size(0), owner(0), sharedPtr2(sharedPtr), sharedPtr(sharedPtr2) { }
    SharedPtrAdapter(      S& sharedPtr) : ptr(0), size(0), owner(0), sharedPtr(sharedPtr) { }
    SharedPtrAdapter(const SC& sharedPtr) : ptr(0), size(0), owner(0), sharedPtr2(std::const_pointer_cast<T>(sharedPtr)), sharedPtr(sharedPtr2) { }
    SharedPtrAdapter(const S* sharedPtr) : ptr(0), size(0), owner(0), sharedPtr(*(S*)sharedPtr) { }

For functions taking either std::shared_ptr<T> or std::shared_ptr<const T> we can already pass the std::shared_ptr<T> resulting from a cast from the adapter.
For functions returning either std::shared_ptr<T> or std::shared_ptr<const T>, we could now create an adapter in both cases.

Am I wrong ?

[saudet]

Should like something that could work...

[HGuillemet]

Could you review that ?
I will generalize to other smart pointers if it looks ok.

We must also find a way to have explicit cast asX() keep the existing shared_ptr.

[HGuillemet]

Either these remove_const, add_const, or we must ensure in adapterInformation that valueTypeName has no const (for all adapters ??).

[saudet]

Could you review that ?
I will generalize to other smart pointers if it looks ok.

Well, did you test it with all presets that use @SharedPtr? The problem I see with this is that it's probably going to break a lot of things...

[HGuillemet]

Are there annotations which could legitimately be duplicated ?

[HGuillemet]

Could you review that ?
I will generalize to other smart pointers if it looks ok.

Well, did you test it with all presets that use @SharedPtr? The problem I see with this is that it's probably going to break a lot of things...

Not yet. I did for Pytorch. I'd like to know if you see something wrong first.
We cannot test easily for sure like we can for Parser-only changes by comparing parser outputs.
What may break existing presets using @sharedptr but not the new share info is the new definition of the adapter. But theoretically it could be able to handle const variations transparently.

[saudet]

Right, so what about moving the development of that to the PyTorch presets? We can add adapters there like this:
https://github.com/bytedeco/javacpp-presets/blob/master/opencv/src/main/resources/org/bytedeco/opencv/include/opencv_adapters.h
And then when everything works for PyTorch, you can start moving things back to SharedPtrAdapter.

[HGuillemet]

My roadmap for pytorch is to propose another PR (yes, again, but it should be the last one :) ) about explicit upcasts (dealing with virtual inheritance), once this one is merged. Then I'll have a Pytorch presets ready with all improvements. It already works well with this PR and the next. I'm currently training models with them.

For this PR, if you're afraid of the changes in the SharedPtrAdapterabout const, we can move it aside. It's not the main part of this PR. It's only here to allow the new share info to work with classes using const in the shared_ptr argument. There is only 1 such class in python preset, and we can still keep the manual @cast @PointerCast @StdShared annotating. But please have a close look at this part to see if the change can really damage anything. I doubt it, but I may be wrong.

[saudet]

We don't need to make any modifications to the core of JavaCPP to do most of what you're proposing for that too. There's a lot we can move to the presets for PyTorch. Why do you want to modify everything here?

[HGuillemet]

Don't you think the ability to create shared objects from Java is a good thing that can benefit to other presets ? Or are you talking about something else ?

[saudet]

Like I said, we can already provide functions like that without modifying anything from the core of JavaCPP. It would be nice if the Generator could pick up annotations on allocate() and call a predetermined static function from the adapter, but you're not doing this here, and the rest of what you're doing can be done without modifying the core of JavaCPP.

[HGuillemet]

It would be nice if the Generator could pick up annotations on allocate() and call a predetermined static function from the adapter,

Did you have a look at the changes ? It does exactly that : when @Adapter is detected on allocate(), it instantiate an adapter instead of the object directly. No need to add a static function to the adapter. Unless I didn't understand what you meant.

[HGuillemet]

Here is an example of what an allocate with @sharedptr looks like:

JNIEXPORT void JNICALL Java_shared_M_allocate__(JNIEnv* env, jobject obj) {
    jthrowable exc = NULL;
    try {
        SharedPtrAdapter< ns::M > radapter(new ns::M(), 1, NULL);
        ns::M* rptr = radapter;
        jlong rcapacity = (jlong)radapter.size;
        void* rowner = radapter.owner;
        void (*deallocator)(void*) = rowner != NULL ? &SharedPtrAdapter< ns::M >::deallocate : 0;
        JavaCPP_initPointer(env, obj, rptr, rcapacity, rowner, deallocator);
    } catch (...) {
        exc = JavaCPP_handleException(env, 8);
    }

    if (exc != NULL) {
        env->Throw(exc);
    }
}

[saudet]

I see, we don't need to change SharedPtrAdapter to wrap on allocate(). So let's start by merging in that modification only.

[HGuillemet]

How do you add an annotation on allocate from an Info ? We need something more in the Parser to allow that.
What seemed the most logical for me is an info on the whole class, meaning "the objects of this class must be managed by smart pointers, don't create unmanaged instances". And if we add an info to annotate all allocate methods, this info might as well automatize the addition of the @Cast @PointerType @SharedPtr formula on the shared_ptr type. You seem to agree with that above, but maybe I misunderstood.

The PR doesn't do much more than that. I'll move out the modification of the adapter about const for now. There is also a modification of asX to replace the static_cast by a static_pointer_cast and return a managed object. Because we don't want to create unmanaged instances. This will be still more useful in the PR about explicit cast since it'll use these asX explicit cast to jump over virtual inheritances.

Now, as said above, I did this as a proof of concept for shared pointer but it needs to be generalized.
We can either:

• generalize to smart pointers. Replacing Info.share with something like info.managed("shared_ptr"), or create other infos like Info.weak, Info.unique
• generalize to any adapter. In this case we may need to add functions to adapters to implement what is done by asX.

SInce I haven't spent time on understanding the roles of other adapters yet and hwo they work, I have no opinion about this.

[saudet]

We probably don't need to add a new field to Info for any of that. Using the Info.annotations given for constructors should be sufficient. As for Type.shared and shared_ptr fields, from what I can see, that's only useful to add the annotation hacks for PyTorch that are very specific to how it makes use of std::shared_ptr, so like I keep telling you, that belongs in the presets of PyTorch, unless we figure out some way to generalize that, yes, sure, but instead of trying to do everything in this single PR, let's first merge things that clearly make sense.

[HGuillemet]

that's only useful to add the annotation hacks for PyTorch that are very specific to how it makes use of std::shared_ptr

Sorry but my experience with presets are limited. What exactly is special in how Pytorch uses std::shared_ptr ?

I'm just realizing that it can just work without any info, I thought that the pointerTypes and annotations(@cast...@sharedptr) was mandatory for the merge of native class X and shared_ptr into a single java class X could work.