| Summary: | Android SDL_rwops not thread-safe(?) | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Ellie <etc0de> |
| Component: | file | Assignee: | Gabriel Jacobo <gabomdq> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | blocker | ||
| Priority: | P2 | CC: | gabomdq, pierre-yves |
| Version: | HG 2.0 | ||
| Hardware: | ARM | ||
| OS: | Android (All) | ||
| See Also: | http://bugzilla.libsdl.org/show_bug.cgi?id=1159 | ||
| Attachments: |
Store JNIEnv locally in each thread
Better thread handling for Android Better thread handling for Android |
||
|
Description
Ellie
2012-02-18 07:16:04 UTC
My concern with the patch in bug 1159 is that it looks like it's attaching and detaching the java environment in each API call, which seemed extremely bad for performance. Does the approach taken there fix the threading issues for you? There are some active Android developers on the mailing list. If you haven't recently broached this there, it might be a good idea to try it and see if anyone has suggestions on how to fix this. I don't do enough JNI programming to know what the best fix is myself. FYI, fixing this may be as simple as using a mutex inside the file functions to protect the data there, and if that's the case, this may be an easy fix. Would a mutex allow only one file access at once? My sound thread keeps some files constantly opened (for streaming) and would therefore block everyone else forever if that was the case. I attempted to use the patch from #1159. It causes a crash when opening a window which I think worked before - probably thanks to the many manual adjustments done by me because half of the patch failed with the current code and I had to guess a lot (and probably guessed wrong). Even if that patch offers just a slow solution, what about adding it to SDL with a compile time switch that is disabled per default so that people who need threading, like me, can enable it explicitely, while all the others keep the performance? I could fiddle around with the patch until maybe I get it to work. Would that be helpful to you? I tried hacking around file access from other threads than the main thread now (in my application code) which resolves the issue. However, if you imagine an SDL sound callback where .ogg is decoded from a file on disk that is constantly streamed, you can imagine what a mess hacking this file access back into the main thread is. Any sane solution, even if involving a performance penalty, would be appreciated. Anything in SDL would be faster (and more reliable) than my current application space attempt of redirecting all file access into the busy main thread and making the other threads wait for the result - which is a code and performance nightmare. I tried a mailing list posting now, so far it hasn't attracted JNI-savvy people: http://forums.libsdl.org/viewtopic.php?t=8076 Have you considered dropping JNI altogether? It could probably resolve this and it should be faster doing things the direct way. Android 2.3 is pretty much standard by now. I filed a separate enhancement bug for this: http://bugzilla.libsdl.org/show_bug.cgi?id=1466 Also I looked at some native NDK code in the Android docs and it looks nicer to me than the huge JNI wrapping stuff SDL is filled with right now. A few comments on this issue from what I've researched... - Calling AttachCurrentThread on an already attached thread is a no op, and is actually the recommended method of getting the thread-exclusive JNIEnv environment. So, the first thing will be removing the static mEnv in SDL_android.cpp and replacing it for a local variable obtained via AttachCurrentThread (this is actually what I think is causing problems right now, at some point a thread is trying to use another threads JNIEnv) - It seems you can have multiple threads attached at the same time, I haven't seen any mention of this being an exclusive sort of operation - Threads have to be detached before they end, and that can be done almost automatically using pthread_key_create...how exactly I don't know :) References: http://developer.android.com/guide/practices/design/jni.html https://groups.google.com/forum/?fromgroups#!topic/android-ndk/vLt3ncT3I4g http://java.sun.com/docs/books/jni/html/invoke.html Sam, let me know your thoughts on this, I can try to create a patch based on this asumptions and see if that solves the threading problems we've been seeing. Created attachment 881 [details]
Store JNIEnv locally in each thread
Can anyone try this patch and see if it solves the problems? Thanks!
*** Bug 1159 has been marked as a duplicate of this bug. *** Created attachment 885 [details]
Better thread handling for Android
Attaching improved patch
Created attachment 887 [details]
Better thread handling for Android
Evgeny Miroshnichenko confirmed this solves the threading issues on Android, this is now changeset http://hg.libsdl.org/SDL/rev/17840f487124 Sorry, university kept me busy. I will check the results the next three days. Thanks for the fix :-) (assuming it will work for me) |