bytedeco / javacpp

The missing bridge between Java and native C++
Other
4.46k stars 581 forks source link

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

Closed blueberry closed 6 years ago

blueberry commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

I'm pretty sure PointerPointer is working properly. I've added a test in commit https://github.com/bytedeco/javacpp/commit/fa35503c768441aad93ed227048d194130ed1c6e and it's working just fine here. Does it fail on your machine?

blueberry commented 6 years ago

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 commented 6 years ago

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

blueberry commented 6 years ago

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 commented 1 year ago

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 :)