Implement basic automatic attachment/detachment of c++ threads to JVM. #405
Conversation
No side effects if not enabled Compile with -DEXPERIMENTAL_AUTO_CPP_THREAD_ATTACH to enable Requires a compiler with C++11 support Borrows code/idea from dropbox#372 (comment)
|
I realise from the various bug reports that you have other possible reservations about using c++ created threads and understand the approach you have taken of using java threads instead; though note that you don't fully enumerate these reservations. When wrapping an existing library/application in djinni, especially a larger more complicated one, redoing all the threading can be quite invasive, and being able to work with the c++ threads - even if just on a temporary basis while developing an initial djinni POC/protoype (with an eventual eye on redoing all the threading at a later point in development cycle if need be) can help lower the difficulty and ease the burden quite a bit. Further having the possibility to test like this can help expose what other possible issues exist (other than attach/detach) if any; and possibly these too can then be addressed, or if not addressed at least better enumerated so that the issue can be better understood. So I think having this available in an optionally enabled way, that makes it clear that its experimental and not the 'supported' way is a good way forward. |
| jint get_res = g_cachedJVM->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6); | ||
| #ifdef EXPERIMENTAL_AUTO_CPP_THREAD_ATTACH | ||
| if (get_res == JNI_EDETACHED) { | ||
| get_res = g_cachedJVM->AttachCurrentThread(&env, nullptr); |
There was a problem hiding this comment.
I ran into a problem compiling the pull request using Oracle's JDK. The code compiles if I use Android's jni.h, but if I use jni.h that comes with Oracle's JDK, it fails because the AttachCurrentThread parameters are different. Android expects JNIEnv** whereas the JDK expects void**. I got around it with ifdefs, though I'm still in the process of testing the change at runtime.
There was a problem hiding this comment.
Also ran into a runtime issue with Android on NDK 16b, but NDK 18b works. Desktop Java (1.8) works as well (if you account for the difference in attach current thread parameters).
There was a problem hiding this comment.
Would it be sufficient to introduce a compile time error for ndk <18?
It seems to me that as an experimental/optional feature thats off by default it should be fine for it to only work on new ndks going forward.
|
Hi, @mjmacleod: |
|
I have now completed the CLA. |
|
I can confirm that this diff has passed all the tests. There are quite a lot of discussions regarding this topic, I'm going through all of them to make sure I have up-to-date knowledge. Will get back to you as soon as I fully understood all the context. |
|
@mjmacleod when I tried your solution I noticed that the deconstruct of the thread_local variable is not called in Android versions less than Android M. Please advise. |
|
android/ndk#687 - relevant ndk bug report regarding this. It is my understanding based on this bug that this is only an issue in older ndks and is fixed in newer ones. Can you confirm which ndk you are using? Given the optional/experimental/opt-in nature of this functionality I think it is acceptable for it to require a recent NDK and not support older ones. |
|
@mjmacleod I'm using NDK16, so Do you think if I upgrade it to NDK18 it will work? I have a lot of experimental/optional in my code so I'm trying to find a workaround instead of the upgrading, so I have tried to update djinni_support.cpp based on this comment: #372 (comment) and I called createThreadKey() in JniClassInitializer::get_all(), and it's working. Where do you think the best place to call it instead of get_all() |
|
I believe that using either 18b or 19 this code should work on pre-M devices. With cpp development for android already being tricky I personally would not want to complicate it further by being on outdated toolkits; so personally I'd strongly consider following the path of least pain and go for the latest NDK, and spend any time on making that work rather than spending time trying to support the old NDK... If upgrading is really not an option I'd strongly consider going the standard recommended djinni route instead of using the define in this patch. Which is to have java handle all thread creation/deletion instead of doing it on the cpp side. (https://github.com/dropbox/djinni/tree/master/extension-libs/platform-threads) If that is also not an option #372 should of course work (posix platforms only), I'm not 100% sure where the best place to call createThreadKey() is in that case, perhaps the original author of #372 can assist or alternatively I'd suggest asking for advice on the "mobile c++" slack, where a great number of people with more specific experience in this can help you. |
dropbox#405) * Implement basic automatic attachment/detachment of c++ threads to JVM. No side effects if not enabled Compile with -DEXPERIMENTAL_AUTO_CPP_THREAD_ATTACH to enable Requires a compiler with C++11 support Borrows code/idea from dropbox#372 (comment) * Place assignment outside of define.
No side effects if not enabled
Compile with -DEXPERIMENTAL_AUTO_CPP_THREAD_ATTACH to enable
Requires a compiler with C++11 support
Borrows code/idea from #372 (comment)