search

dropbox / djinni

A tool for generating cross-language type declarations and interface bindings.
Apache License 2.0
2.88k stars 487 forks source link

Implement basic automatic attachment/detachment of c++ threads to JVM. #405

Closed mjmacleod closed 5 years ago

mjmacleod commented 6 years ago

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 https://github.com/dropbox/djinni/issues/372#issuecomment-394629525

mjmacleod commented 6 years ago

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.

xianwen commented 5 years ago

Hi, @mjmacleod: I noticed you haven't signed the CLA yet, could you please sign it here: https://opensource.dropbox.com/cla/ Thanks a lot!

mjmacleod commented 5 years ago

I have now completed the CLA.

xianwen commented 5 years ago

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.

MouhamadKawas commented 5 years ago

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

mjmacleod commented 5 years ago

@MouhamadKawas

https://github.com/android-ndk/ndk/issues/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. Assuming an NDK upgrade resolves this introducing an informative compiler error message informing minimum NDK requirements might be a pragmatic solution to this.

MouhamadKawas commented 5 years ago

@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: https://github.com/dropbox/djinni/issues/372#issue-329040767

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

mjmacleod commented 5 years ago

@MouhamadKawas

I believe that using either 18b or 19 this code should work on pre-M devices. The compilers in NDK16 are very old and have some very bad quirks in terms of using strange stdlib implementations with odd behaviour etc.

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.