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

MKL-DNN How to create an array of mkldnn_primitive needed by mkldnn_stream_submit #251

Closed
blueberry opened this issue Aug 18, 2018 · 13 comments
Labels

Comments

@blueberry
Copy link

blueberry commented Aug 18, 2018

I spent a lot of time and tried to wrestle basic javacpp.Pointer to do this, but without success by now.

in MKL-DNN C api, there is the following method:

mkldnn_stream_submit(mkldnn.mkldnn_stream stream, long n, mkldnn.mkldnn_primitive primitives, mkldnn.mkldnn_primitive error_primitive)

When i try it with n = 1 and a single primitive, that works well.

However, the function is clearly intended to accept n > 1 and an array of primitives. This is also what examples provided in the mkl-dnn github repository show. The trouble is: in C, the type of the element and the type of the array are the same. I can simply provide a pointer to the first element, and the api will use the information contained in the n argument to access the whole array.

In Java, I'd expect the method to either accept a mkldnn_primitive array, or mkldnn_primitive to support creating a pointer to the native array of its type. However, unlike other pointer types such as mkldnn_primitive_at_t, which do offer constructors that support size, and methods such as position and put that can be used to fill in the array, mkldnn_primitive does not.

Is this intentional, or an oversight?

If it is intentional, what is the recommended way of building array arguments for methods such as mkldnn_stream_submit?

@saudet saudet added the bug label Aug 23, 2018
@saudet
Copy link
Member

saudet commented Aug 23, 2018

Sorry for the late answer, forgot about this issue. mkldnn_primitive_t is an opaque pointer type, so declarations like mkldnn_primitive_t net_fwd[10] are actually pointers to pointers, so we'll need some overloads with PointerPointer...

@blueberry
Copy link
Author

FWIF, I tried using PointerPointer, but, while it does compile and run, it crashes the JVM (without the core dump if I remember well!). I guess it's because the size of the generic pointer structure and the size of mkldnn_primitive_t is different...

saudet added a commit to bytedeco/javacpp-presets that referenced this issue Aug 23, 2018
@saudet
Copy link
Member

saudet commented Aug 23, 2018

We need to overload the methods for this to work. I've just pushed a commit for that, so it should work now.

Thanks for reporting!

@blueberry
Copy link
Author

Thanks for looking into this. This was even a much bigger issue than I reported at first. The reported issue had a workaround, but the same issue bites in mkldnn_primitive_create, which might expect more than one output when the algo_kind is forward_training. In that case, it expect both dst_memory and workspace_memory as outputs. The current API didn't have support for this.

I'll try the latest snapshots and see whether this issue has been solved.

However, there is a new issue or, rather, annoyance, with the latest changes: now PointerPointer overload has been introduced as an alternative even in places where this does not make sense. For example, the method mkldnn_primitive_desc_create_v2 or mkldnn_primitive_desc_create accept PointerPointer as a first argument when such operation is clearly intended to work on exactly one pointer, and will, I suppose, initialize only the first mkldnn_primitive in that array...

@blueberry
Copy link
Author

blueberry commented Aug 23, 2018

@saudet Looking at the changes in the code, I am not sure how this is supposed to be used. I get the overloads, that part is clear: the methods now accept PointerPointer. But I am not sure what is the right way to create that PointerPointer with specific types such as mkldnn_primitive. Should vanilla PointerPointer methods work as-is with mkldnn_primitive? Basically, if I create a PointerPointer, and fill its positions with mkldnn_primitive's, I am doing it like it is supposed to be done?

@blueberry
Copy link
Author

blueberry commented Aug 23, 2018

Reply to future self and the posterity: yes, vanilla PointerPointer works in this use case. Nope, I didn't try it in that test with arrays. Actually, it crashes the VM...

@saudet Should I open a new issue for the "polluted" overloads and close this issue, leave it to be resolved in this issue, or it is altogether not considered a problem?

@blueberry
Copy link
Author

@saudet It turns out I couldn't make it work with PointerPointer. It crashes the VM:

Stack: [0x00007fda4afb7000,0x00007fda4b0b8000],  sp=0x00007fda4b0b5240,  free space=1016k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C  [libmkldnn.so.0+0x1a0f9a]  mkldnn::impl::stream_eager_t::submit_impl(unsigned long, unsigned long, mkldnn_primitive**)+0xba

Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
j  org.bytedeco.javacpp.mkldnn.mkldnn_stream_submit(Lorg/bytedeco/javacpp/mkldnn$mkldnn_stream;JLorg/bytedeco/javacpp/PointerPointer;Lorg/bytedeco/javacpp/PointerPointer;)I+0

This is a pseudocode for what I did (I did it from Clojure, so I don't have the actual Java example to post):

int cnt = 10;
mkldnn_primitive arr[cnt] = <make an array, list or whatever of mkldnn_primitive's that were previously initialized>
PointerPointer pp = new PointerPointer(cnt);
for (i = 0; i < cnt; i++) {
   pp.put(i, arr[i]);
}
pp.position(0); // just in case, although it is 0 anyway...

mkldnn.mkldnn_stream_submit(stream, cnt, pp, null); //<-- JVM crashes! 

I am not sure whether I used PointerPointer as it is intended or there is something else that I should do?

The rest of the code works fine when I submit mkldnn_primitives one by one. Of course, there is still the much more important issue of array of primitives in forward training, that does not have a workaround...

@blueberry
Copy link
Author

blueberry commented Aug 23, 2018

OK, new note for posterity: pp.put(i, p) does not work, but pp.position(i).put(p) does!

I am not sure whether this is a bug or I didn't get quite well the difference between these two flawors of put.
@saudet can you confirm what I've found out by now, and do you have any further hints?

@saudet
Copy link
Member

saudet commented Aug 24, 2018

I'm pretty sure PointerPointer is working properly. I've added a test in commit fa35503 and it's working just fine here. Does it fail on your machine?

@blueberry
Copy link
Author

blueberry commented Aug 24, 2018

I am sure that vanilla PointerPointer works. Here we have two additional things:

  1. It's a PointerPointer of mkldnn_primitive
  2. The place that crashes the JVM is mkldnn_stream_submit (and possibly other methods), not PointerPointer's methods themselves.

@saudet
Copy link
Member

saudet commented Aug 24, 2018

Ok, I'm pretty sure it's unrelated to JavaCPP, but you could create a code snippet that fails?

@blueberry
Copy link
Author

blueberry commented Aug 24, 2018

I could, but that would require writing at least a few hundreds of lines of verbose Java to recreate a minimal example, so not really :) (I'm doing this in Clojure) I wanted to edit the original example that you provided https://github.com/bytedeco/javacpp-presets/tree/master/mkl-dnn, but it is using the C++ API that have special primitive_vector to cover stream submission, not the C API.

Anyway, I can consider the issue that I had resolved and I hope that other people will either not dig that deep into the C API or will find this if they do.

@blueberry
Copy link
Author

blueberry commented Aug 30, 2023

Reply to future self and the posterity: yes, vanilla PointerPointer works in this use case. Nope, I didn't try it in that test with arrays. Actually, it crashes the VM...
image

It is indeed 2023. It is future me, and this thread helped me solve a similar problem with updated DNNL! :)
BTW PointerPoiner works, it's just more convoluted than in an ideal world :)

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

No branches or pull requests

2 participants