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.

Sign up for GitHub

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

Jump to bottom

[jni] Add a call to jvm.DetachCurrentThread when the thread detaches #1163

Merged
merged 14 commits into from
May 22, 2024
Merged

[jni] Add a call to jvm.DetachCurrentThread when the thread detaches #1163

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
16a2c55
Add a call to jvm.DetachCurrentThread when the thread detaches
HosseinYousefi May 21, 2024
5e51399
Fix windows implementation
HosseinYousefi May 21, 2024
70bd232
Add changelog
HosseinYousefi May 21, 2024
97c26a8
Fix windows again
HosseinYousefi May 21, 2024
de72f97
Merge branch 'detach-current-thread' of https://github.com/dart-lang/…
HosseinYousefi May 21, 2024
33e7ab9
Fix windows yet another time!
HosseinYousefi May 21, 2024
9fa7a46
Extern tlsKeys
HosseinYousefi May 21, 2024
b9f160d
Do TlsAlloc/TlsFree in DllMain
HosseinYousefi May 21, 2024
fa07126
Unify the ifdef styles
HosseinYousefi May 21, 2024
cfe3be5
Using TlsAlloc is not needed in Windows. Assume anything not _Win32 t…
HosseinYousefi May 21, 2024
35f59e9
Oops, fix build
HosseinYousefi May 21, 2024
cccad1f
Pass the reference (`&jniEnv`) to `pthread_setspecific`
HosseinYousefi May 21, 2024
8061d8b
Address comments
HosseinYousefi May 22, 2024
4f04ef0
Run clang-format
HosseinYousefi May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkgs/jni/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 0.9.3-wip

- Added lifetime event handling for the thread-local JNI env. Now
`jvm.DetachNativeThread` is called when the thread detaches as recommended
[here](https://developer.android.com/training/articles/perf-jni#threads).

## 0.9.2

- Bumped `minSdk` to 21.
Expand Down
2 changes: 1 addition & 1 deletion pkgs/jni/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

name: jni
description: A library to access JNI from Dart and Flutter that acts as a support library for package:jnigen.
version: 0.9.2
version: 0.9.3-wip
repository: https://github.com/dart-lang/native/tree/main/pkgs/jni

topics:
Expand Down
12 changes: 11 additions & 1 deletion pkgs/jni/src/dartjni.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

#include "dartjni.h"

#ifndef _WIN32
pthread_key_t tlsKey;
#endif

void initAllLocks(JniLocks* locks) {
init_lock(&locks->classLoadingLock);
}
Expand Down Expand Up @@ -146,6 +150,11 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL, // handle to DLL module
// Return FALSE to fail DLL load.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sidenote: This DllMain is assuming the C code in package:jni gets compiled into it's own DLL (?). That would prevent statically linking it into a bigger library (which may have already a different DllMain). It's a little bit of a smell I think.

If we can use C++ here (if not: why?) then a global C++ object can do things in it's constructor and destructor (e.g. initializer locks and free them, create TLS keys, free them, ...) that get run on shared library load & unload times.

(Arguably this doesn't solve the issue of detaching the thread from java at exit)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use C++. We would have to wrap the functions we want to call from Dart in extern "C" but otherwise it should not be a problem. We also generate another C file from jni.h which wraps the API and does things like conversion to global refs and exception checking.

In general, I did not really refactor the C code we had since 2 years ago. I think it's worth it to open an issue and do this in the near future. #1169

InitializeCriticalSection(&spawnLock);
break;
case DLL_THREAD_DETACH:
if (jniEnv) {
HosseinYousefi marked this conversation as resolved.
Show resolved Hide resolved
detach_thread(jniEnv);
}
break;
case DLL_PROCESS_DETACH:
// Perform any necessary cleanup.
DeleteCriticalSection(&spawnLock);
Expand Down Expand Up @@ -771,6 +780,7 @@ Dart_FinalizableHandle newJObjectFinalizableHandle(Dart_Handle object,
return Dart_NewFinalizableHandle_DL(object, reference, 0,
finalizeWeakGlobal);
}
return NULL; // Never happens.
}

FFI_PLUGIN_EXPORT
Expand All @@ -782,7 +792,7 @@ Dart_FinalizableHandle newBooleanFinalizableHandle(Dart_Handle object,
FFI_PLUGIN_EXPORT
void deleteFinalizableHandle(Dart_FinalizableHandle finalizableHandle,
Dart_Handle object) {
return Dart_DeleteFinalizableHandle_DL(finalizableHandle, object);
Dart_DeleteFinalizableHandle_DL(finalizableHandle, object);
}

jclass _c_Object = NULL;
Expand Down
49 changes: 32 additions & 17 deletions pkgs/jni/src/dartjni.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,20 @@
#include <stdio.h>
#include <stdlib.h>

#if _WIN32
#ifdef _WIN32
#include <windows.h>
#else
#include <pthread.h>
#include <unistd.h>
#endif

#if _WIN32
#ifdef _WIN32
#define FFI_PLUGIN_EXPORT __declspec(dllexport)
#else
#define FFI_PLUGIN_EXPORT
#endif

#if defined _WIN32
#ifdef _WIN32
#define thread_local __declspec(thread)
#else
#define thread_local __thread
HosseinYousefi marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -43,7 +43,7 @@

/// Locking functions for windows and pthread.

#if defined _WIN32
#ifdef _WIN32
#include <windows.h>

typedef CRITICAL_SECTION MutexLock;
Expand Down Expand Up @@ -85,8 +85,7 @@ static inline void free_mem(void* mem) {
CoTaskMemFree(mem);
}

#elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \
defined __GNUC__
#else
#include <pthread.h>

typedef pthread_mutex_t MutexLock;
Expand Down Expand Up @@ -127,11 +126,6 @@ static inline void destroy_cond(ConditionVariable* cond) {
static inline void free_mem(void* mem) {
free(mem);
}

#else

#error "No locking/condition variable support; Possibly unsupported platform"

#endif

typedef struct CallbackResult {
Expand Down Expand Up @@ -164,6 +158,33 @@ extern thread_local JNIEnv* jniEnv;

extern JniContext* jni;

/// Handling the lifetime of thread-local jniEnv.
#ifndef _WIN32
extern pthread_key_t tlsKey;
#endif

static inline void detach_thread(void* data) {
jniEnv = NULL;

if (*jni->jvm) {
(*jni->jvm)->DetachCurrentThread(jni->jvm);
}

#ifndef _WIN32
pthread_key_delete(tlsKey);
HosseinYousefi marked this conversation as resolved.
Show resolved Hide resolved
#endif
}

static inline void attach_thread() {
if (!jniEnv) {
HosseinYousefi marked this conversation as resolved.
Show resolved Hide resolved
(*jni->jvm)->AttachCurrentThread(jni->jvm, __ENVP_CAST & jniEnv, NULL);
#ifndef _WIN32
pthread_key_create(&tlsKey, detach_thread);
HosseinYousefi marked this conversation as resolved.
Show resolved Hide resolved
pthread_setspecific(tlsKey, &jniEnv);
#endif
}
}

/// Types used by JNI API to distinguish between primitive types.
enum JniType {
booleanType = 0,
Expand Down Expand Up @@ -278,12 +299,6 @@ FFI_PLUGIN_EXPORT jobject GetApplicationContext(void);
/// Returns current activity of the app on Android.
FFI_PLUGIN_EXPORT jobject GetCurrentActivity(void);

static inline void attach_thread() {
if (jniEnv == NULL) {
(*jni->jvm)->AttachCurrentThread(jni->jvm, __ENVP_CAST & jniEnv, NULL);
}
}

/// Load class into [cls] using platform specific mechanism
static inline void load_class_platform(jclass* cls, const char* name) {
#ifdef __ANDROID__
Expand Down
Loading