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.

Sign up for GitHub

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

Add support for @Name on allocators #706

Closed
wants to merge 4 commits into from
Closed

Add support for @Name on allocators #706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a995e3e
Add support for `@Name` on allocators
HGuillemet Aug 28, 2023
4f3ec6b
Fix generation of allocators with adapters
HGuillemet Aug 28, 2023
e21e54d
Update test with `@Name`
HGuillemet Aug 28, 2023
509e2a3
Revert import glob
HGuillemet Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 76 additions & 16 deletions src/main/java/org/bytedeco/javacpp/annotation/Adapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,82 @@
import org.bytedeco.javacpp.tools.Generator;

/**
* Specifies a C++ class to act as an adapter to convert the types of arguments.
* Three such C++ classes made available by {@link Generator} are {@code StringAdapter},
* {@code VectorAdapter}, and {@code SharedPtrAdapter} to bridge a few differences between
* {@code std::string} and {@link String}; between {@code std::vector}, Java arrays of
* primitive types, {@link Buffer}, and {@link Pointer}; and between {@code xyz::shared_ptr}
* and {@link Pointer}. Adapter classes must define the following public members:
* <ul>
* <li> A constructor accepting 3 arguments (or more if {@link #argc()} > 1): a pointer, a size, and the owner pointer
* <li> Another constructor that accepts a reference to the object of the other class
* <li> A {@code static void deallocate(owner)} function
* <li> Overloaded cast operators to both types, for references and pointers
* <li> A {@code void assign(pointer, size, owner)} function
* <li> A {@code size} member variable for arrays accessed via pointer
* </ul>
* To reduce further the amount of coding, this annotation can also be used on
* other annotations, such as with {@link StdString}, {@link StdVector}, and {@link SharedPtr}.
* Specifies a C++ class to act as an adapter between a target type and one or more adaptee type(s).
* Instances of the adapter class are short-living and last only for the duration of a JNI call.
* <p></p>
* Six such C++ classes are made available by {@link Generator}:
* <blockquote>
* <table width="80%">
* <thead>
* <tr>
* <th>Adapter class</th><th>Target type</th><th>Adaptee types</th><th>Helper annotation</th>
* </tr>
* </thead>
* <tbody>
* <tr>
* <td valign="top">{@code VectorAdapter<P,T,A>}</td>
* <td valign="top">{@code std::vector<T,A>}</td>
* <td valign="top">{@code P}</td>
* <td valign="top">{@link StdVector}</td>
* </tr>
* <tr>
* <td valign="top">{@code StringAdapter<T>}</td>
* <td valign="top">{@code std::basic_string<T>}</td>
* <td valign="top">{@code char}<br>{@code signed char}<br>{@code unsigned char}<br>{@code wchar_t}<br>{@code unsigned short}<br>{@code signed int}</td>
* <td valign="top">{@link StdString}</td>
* </tr>
* <tr>
* <td valign="top">{@code SharedPtrAdapter<T>}</td>
* <td valign="top">{@code SHARED_PTR_NAMESPACE::shared_ptr<T>}</td>
* <td valign="top">{@code T}</td>
* <td valign="top">{@link SharedPtr}</td>
* </tr>
* <tr>
* <td valign="top">{@code UniquePtrAdapter<T,D>}</td>
* <td valign="top">{@code UNIQUE_PTR_NAMESPACE::unique_ptr<T,D>}</td>
* <td valign="top">{@code T}</td>
* <td valign="top">{@link UniquePtr}</td>
* </tr>
* <tr>
* <td valign="top">{@code MoveAdapter<T,D>}</td>
* <td valign="top">{@code T}</td>
* <td valign="top">{@code T}</td>
* <td valign="top">{@link StdMove}</td>
* </tr>
* <tr>
* <td valign="top">{@code OptionalAdapter<T>}</td>
* <td valign="top">{@code OPTIONAL_NAMESPACE::optional<T>}</td>
* <td valign="top">{@code T}</td>
* <td valign="top">{@link Optional}</td>
* </tr>
* </tbody>
* </table>
* </blockquote>
* The helper annotations are shortcuts that infer the template type(s) of the adapter class from the Java
* class they annotate.
* <p></p>
* When an argument of a method is annotated, an instance of the adapter class is created from
* the Java object passed as argument, and this instance is passed to the C++ function, thus triggering
* an implicit cast to the type expected by the function (usually a reference or pointer to the target type).
* If the argument is also annotated with {@link Cast},
* the adapter instance is cast to the type(s) specified by the {@link Cast} annotation before being passed
* to the function.
* <p></p>
* When a method is annotated, an instance of the adapter is created from the
* value (usually a pointer or reference to the target type) returned by the C++ function or by {@code new} if the method is an allocator.
* If the method is also annotated with {@link Cast}, the value returned by the C++ function is
* cast by value 3 of the {@link Cast} annotation, if any, before instantiation of the adapter.
* Then a Java object is created from the adapter to be returned by the method.
* <p></p>
* Adapter classes must at least define the following public members:
* <ul>
* <li> For each adaptee type, a constructor accepting 3 arguments (or more if {@link #argc()} > 1): a pointer to a const value of the adaptee, a size, and the owner pointer
* <li> Another constructor that accepts a reference to the target type
* <li> A {@code static void deallocate(owner)} function
* <li> Overloaded cast operators to both the target type and the adaptee types, for references and pointers
* <li> {@code void assign(pointer, size, owner)} functions with the same signature than the constructors accepting 3 arguments
* <li> A {@code size} member variable for arrays accessed via pointer
* </ul>
*
* @see Generator
*
Expand Down
23 changes: 12 additions & 11 deletions src/main/java/org/bytedeco/javacpp/tools/Generator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2645,11 +2645,16 @@ void call(MethodInformation methodInfo, String returnPrefix, boolean secondCall)
prefix = "";
suffix = "";
} else {
out.print((noException(methodInfo.cls, methodInfo.method) ?
"new (std::nothrow) " : "new ") + valueTypeName + typeName[1]);
if (methodInfo.arrayAllocator) {
prefix = "[";
suffix = "]";
if (methodInfo.method.isAnnotationPresent(Name.class)) {
out.print(methodInfo.memberName[0]);
// If method is an array allocator, the function must return a pointer to an array
} else {
out.print((noException(methodInfo.cls, methodInfo.method) ?
"new (std::nothrow) " : "new ") + valueTypeName + typeName[1]);
if (methodInfo.arrayAllocator) {
prefix = "[";
suffix = "]";
}
}
}
} else if (Modifier.isStatic(methodInfo.modifiers) || !Pointer.class.isAssignableFrom(methodInfo.cls)) {
Expand Down Expand Up @@ -2814,12 +2819,8 @@ void returnAfter(MethodInformation methodInfo) {
// special considerations for std::string without adapter
out.print(");\n" + indent + "rptr = rstr.c_str()");
}
if (adapterInfo != null) {
if (methodInfo.allocator || methodInfo.arrayAllocator) {
suffix = ", 1, NULL)" + suffix;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to remove this? I don't see how that's not going to break anything

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reversal to pre-PR#668.
The only thing it'll break is the new @SharedPtr annotation on constructor, that will now need the @Name annotation too. But I'm pretty sure only the pytorch presets uses that for now.

Copy link
Contributor Author

@HGuillemet HGuillemet Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More explanations: @SharedPtr on a method means: the native function returns a shared_ptr, construct a Java object containing the shared_ptr in owner, by instantiating an adapter from this shared_ptr.

But for PR #668 I had to change the logic since new, called by allocate returned a plain pointer. So the meaning of @SharedPtr on allocators was different: the native function returns a plain pointer, turn it into a shared pointer, by instantiating an adapter from the plain pointer (thus the ", 1, NULL)" additional arguments), and build a java object containing it.

This worked, but in addition to altering the logic and requiring these lines in generator to implement the exception to the logic, it prevented us to use @SharedPtr @Name("std::dynamic_pointer_cast<DerivedClass*>"), since dynamic_pointer_cast returns a shared pointer, not a plain pointer.

So I suggest to revert to the common logic where @SharedPtr always means that the native function returns a shared_ptr and explicitly build the shared_ptr by replacing new by make_shared using @Name, instead of delegating this to the adapter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds alright, but let's try not to change things back and forth like that all the time. I'm assuming you might want to change that a bit further once you start testing this with pull #700, so could you just put all that there and start testing everything together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already tested both PR on pytorch and it's working well. Do you prefer that I merge both PR into #700 and close this one ? Or that I rebase #700 on this branch ?

Copy link
Contributor Author

@HGuillemet HGuillemet Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that we might want to add before merging is support for array allocator.

About @Name (this PR): see Note 1 above. I'm not 100% sure this is the right choice. An alternative would be to make a loop and call the function for each element. We should then allocate with malloc instead of C++ array allocator to prevent the call to the constructors. (won't work). Any opinion ?

About adapters: I think JNI just doesn't compile if placed on allocateArray.

Anyway, currently, when we add an annotation on constructor with an Info, Parser doesn't place it on allocateArray. But that is close to a bug, since for instance, if someone ever calls:

ReLUImpl relus = new ReLUImpl(10);

Each ReLUImpl will not have been created with make_shared, Potentially leading to memory corruption.

EDIT: In fact it seems impossible to make shared pointers work with array allocator, since owner will be the same for all elements of the array. So if we pass element 4 of the relus array defined above to a function taking a shared_ptr<Module>, the function will received the shared pointer to element 0. Am I wrong ? If not, I guess we must deactivate array allocators for classes using shared pointers.

Copy link
Contributor Author

@HGuillemet HGuillemet Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see Note 2. A possible improvement is to find a way to use a predictable name for the proxy class of a virtualized class. Any idea about this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already tested both PR on pytorch and it's working well. Do you prefer that I merge both PR into #700 and close this one ? Or that I rebase #700 on this branch ?

Well, pull #700 doesn't use @Name so it's not complete either. Let's have it all in one place, doesn't matter where I guess

One thing that we might want to add before merging is support for array allocator.

If you want, but please don't try to modify Info just for something that probably no one is going to use

lso see Note 2. A possible improvement is to find a way to use a predictable name for the proxy class of a virtualized class. Any idea about this ?

It's predictable enough, it probably just needs to be documented, or maybe a way to configure it, possibly with Info.cppTypes, which I think isn't being currently used for anything in the case of classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll merge this into #700.

Concerning arrayAllocators: do you agree with the fact that shared pointer adapter just cannot work with arrays of objects or did I miss something ? In this case my preference would be to disable the generation of array allocators for classes for which an annotation has been placed on the info of the constructor (instead of just ignoring the info like it's the case currently). No need for additional Info.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, until we can think of something useful to do, we can skip allocateArray in that case.

} else if (!methodInfo.returnType.isPrimitive()) {
suffix = ")" + suffix;
}
if ((methodInfo.allocator || methodInfo.arrayAllocator || !methodInfo.returnType.isPrimitive()) && adapterInfo != null) {
suffix = ")" + suffix;
}
if ((Pointer.class.isAssignableFrom(methodInfo.returnType) ||
(methodInfo.returnType.isArray() &&
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/org/bytedeco/javacpp/AdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.bytedeco.javacpp.annotation.Cast;
import org.bytedeco.javacpp.annotation.Const;
import org.bytedeco.javacpp.annotation.Function;
import org.bytedeco.javacpp.annotation.Name;
import org.bytedeco.javacpp.annotation.Optional;
import org.bytedeco.javacpp.annotation.Platform;
import org.bytedeco.javacpp.annotation.SharedPtr;
Expand Down Expand Up @@ -82,7 +83,7 @@ public class AdapterTest {
static class SharedData extends Pointer {
SharedData(Pointer p) { super(p); }
SharedData(int data) { allocate(data); }
@SharedPtr native void allocate(int data);
@SharedPtr @Name("std::make_shared<SharedData>") native void allocate(int data);

native int data(); native SharedData data(int data);
}
Expand Down