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

Add support for @Name on allocators #706

Closed
wants to merge 4 commits into from

Conversation

HGuillemet
Copy link
Contributor

@HGuillemet HGuillemet commented Aug 28, 2023

Add support for @Name on allocate() and allocateArray().
The functions passed as parameter to @Name will replace the call to new.

This will be used for instance in (see PR #668):

@SharedPtr @Name("std::make_shared<xxx>") private native void allocate();

or (see PR #700):

@Name("std::dynamic_cast<DerivedClass*>") private native void allocate(BaseClass pointer);

Note 1: if placed on a array allocator, the function is supposed to return a pointer to an array. For instance resulting from std::make_shared<xxx>(n) (unfortunately supported since C++20 only).

Note 2: in the special case of classes with @Virtual methods, the class to instantiate is the proxy class, with its mangled name, so we need to write things like:

@SharedPtr @Name("std::make_shared<JavaCPP_torch_0003a_0003ann_0003a_0003aModule>") private native void allocate();

which is a bit hacky, but works.

Also included in the PR is an updated and more detailed javadoc comment for @Adapter.

@@ -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

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

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

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.

@HGuillemet
Copy link
Contributor Author

This PR was merged into #700.

@HGuillemet HGuillemet closed this Sep 12, 2023
@HGuillemet HGuillemet deleted the name_on_allocate branch January 1, 2024 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants