From 944200b5a9705a7617f82cdaf5caf8932380aba4 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Fri, 6 Oct 2023 09:17:35 +0200 Subject: [PATCH] JNI: Reduce amount of temporary QJniEnvironment instantiations Almost all operations on a QJniObject require a QJniEnvironment, including the construction and destruction of a QJniObject. Instead of instantiating a temporary QJniEnvironment object in each call, store the one from the constructor in the private, and reuse it. Pass the stored environment through to other functions needing it, and add a checkAndClearExceptions() wrapper. Static class members still need their own QJniEnvironment, but we can reuse the one we have to get both jclass and jmethodID rather than creating new QJniEnvironments in several wrappers. As a drive-by, clean up nullptr usage in the test that failed when shortcutting isSameObject for the trivial cases. Change-Id: Ibadbd2be8a0ec9ab62daf285608ee7fe0a3c8852 Reviewed-by: Assam Boudjelthia --- src/corelib/kernel/qjniobject.cpp | 98 ++++++++++++------- src/corelib/kernel/qjniobject.h | 64 +++++++----- .../kernel/qjniobject/tst_qjniobject.cpp | 2 +- 3 files changed, 103 insertions(+), 61 deletions(-) diff --git a/src/corelib/kernel/qjniobject.cpp b/src/corelib/kernel/qjniobject.cpp index d96ad7e945..5a5239fc63 100644 --- a/src/corelib/kernel/qjniobject.cpp +++ b/src/corelib/kernel/qjniobject.cpp @@ -277,8 +277,19 @@ using namespace Qt::StringLiterals; class QJniObjectPrivate { public: - QJniObjectPrivate() = default; + // This is safe and necessary - the JNIEnv is attached to the current thread + // and the pointer remains valid for as long as the thread is alive. And if the + // QJniObject outlives the thread that it lives in, and gets called for + // anything other than destruction, then we have a data race anyway. + // And it's necessary, because the QJniEnvironment destructor calls + // checkAndClearExceptions, and since we have QJniObjects that get destroyed from + // a different thread than the one "owning" it, this triggers JNI assertions. + QJniObjectPrivate() + : m_env(QJniEnvironment().jniEnv()) + { + } ~QJniObjectPrivate() { + // use the environment of the current thread here QJniEnvironment env; if (m_jobject) env->DeleteGlobalRef(m_jobject); @@ -286,10 +297,16 @@ public: env->DeleteGlobalRef(m_jclass); } + JNIEnv *jniEnv() const noexcept + { + return m_env; + } + template - void construct(JNIEnv *env, const char *signature = nullptr, Args &&...args) + void construct(const char *signature = nullptr, Args &&...args) { if (m_jclass) { + JNIEnv *env = jniEnv(); // get default constructor jmethodID constructorId = QJniObject::getCachedMethodID(env, m_jclass, m_className, "", signature ? signature : "()V"); @@ -307,10 +324,11 @@ public: } } + QByteArray m_className; + JNIEnv * const m_env; jobject m_jobject = nullptr; jclass m_jclass = nullptr; bool m_own_jclass = true; - QByteArray m_className; }; template @@ -337,13 +355,12 @@ static jclass getCachedClass(const QByteArray &className) exception clearing and delete the local reference before returning. The JNI object can be null if there was an exception. */ -static QJniObject getCleanJniObject(jobject object) +static QJniObject getCleanJniObject(jobject object, JNIEnv *env) { if (!object) return QJniObject(); - QJniEnvironment env; - if (env.checkAndClearExceptions()) { + if (QJniEnvironment::checkAndClearExceptions(env)) { env->DeleteLocalRef(object); return QJniObject(); } @@ -392,7 +409,7 @@ jclass QtAndroidPrivate::findClass(const char *className, JNIEnv *env) } if (!clazz) { - // We didn't get an env. pointer or we got one with the WRONG class loader... + // Wrong class loader, try our own QJniObject classLoader(QtAndroidPrivate::classLoader()); if (!classLoader.isValid()) return nullptr; @@ -571,12 +588,11 @@ QJniObject::QJniObject() QJniObject::QJniObject(const char *className) : d(new QJniObjectPrivate()) { - QJniEnvironment env; d->m_className = className; - d->m_jclass = loadClass(d->m_className, env.jniEnv()); + d->m_jclass = loadClass(d->m_className, jniEnv()); d->m_own_jclass = false; - d->construct(env.jniEnv()); + d->construct(); } /*! @@ -595,14 +611,13 @@ QJniObject::QJniObject(const char *className) QJniObject::QJniObject(const char *className, const char *signature, ...) : d(new QJniObjectPrivate()) { - QJniEnvironment env; d->m_className = className; - d->m_jclass = loadClass(d->m_className, env.jniEnv()); + d->m_jclass = loadClass(d->m_className, jniEnv()); d->m_own_jclass = false; va_list args; va_start(args, signature); - d->construct(env.jniEnv(), signature, args); + d->construct(signature, args); va_end(args); } @@ -635,12 +650,11 @@ QJniObject::QJniObject(const char *className, const char *signature, ...) QJniObject::QJniObject(jclass clazz, const char *signature, ...) : d(new QJniObjectPrivate()) { - QJniEnvironment env; if (clazz) { - d->m_jclass = static_cast(env->NewGlobalRef(clazz)); + d->m_jclass = static_cast(jniEnv()->NewGlobalRef(clazz)); va_list args; va_start(args, signature); - d->construct(env.jniEnv(), signature, args); + d->construct(signature, args); va_end(args); } } @@ -691,7 +705,7 @@ QJniObject::QJniObject(jobject object) if (!object) return; - QJniEnvironment env; + JNIEnv *env = d->jniEnv(); d->m_jobject = env->NewGlobalRef(object); jclass cls = env->GetObjectClass(object); d->m_jclass = static_cast(env->NewGlobalRef(cls)); @@ -721,6 +735,13 @@ QJniObject::QJniObject(jobject object) QJniObject::~QJniObject() {} +/*! \internal +*/ +JNIEnv *QJniObject::jniEnv() const noexcept +{ + return d->jniEnv(); +} + /*! \fn template T QJniObject::object() const @@ -773,7 +794,7 @@ jclass QJniObject::objectClass() const QByteArray QJniObject::className() const { if (d->m_className.isEmpty() && d->m_jclass && d->m_jobject) { - QJniEnvironment env; + JNIEnv *env = jniEnv(); if (env->PushLocalFrame(3) != JNI_OK) // JVM out of memory return d->m_className; jmethodID mid = env->GetMethodID(d->m_jclass, "getClass", "()Ljava/lang/Class;"); @@ -934,12 +955,11 @@ QByteArray QJniObject::className() const */ QJniObject QJniObject::callObjectMethod(const char *methodName, const char *signature, ...) const { - QJniEnvironment env; - jmethodID id = getCachedMethodID(env.jniEnv(), methodName, signature); + jmethodID id = getCachedMethodID(jniEnv(), methodName, signature); if (id) { va_list args; va_start(args, signature); - QJniObject res = getCleanJniObject(env->CallObjectMethodV(d->m_jobject, id, args)); + QJniObject res = getCleanJniObject(jniEnv()->CallObjectMethodV(d->m_jobject, id, args), jniEnv()); va_end(args); return res; } @@ -972,7 +992,7 @@ QJniObject QJniObject::callStaticObjectMethod(const char *className, const char if (id) { va_list args; va_start(args, signature); - QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args)); + QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args), env.jniEnv()); va_end(args); return res; } @@ -990,13 +1010,13 @@ QJniObject QJniObject::callStaticObjectMethod(const char *className, const char QJniObject QJniObject::callStaticObjectMethod(jclass clazz, const char *methodName, const char *signature, ...) { - QJniEnvironment env; if (clazz) { + QJniEnvironment env; jmethodID id = getMethodID(env.jniEnv(), clazz, methodName, signature, true); if (id) { va_list args; va_start(args, signature); - QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args)); + QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, id, args), env.jniEnv()); va_end(args); return res; } @@ -1022,11 +1042,11 @@ QJniObject QJniObject::callStaticObjectMethod(jclass clazz, const char *methodNa */ QJniObject QJniObject::callStaticObjectMethod(jclass clazz, jmethodID methodId, ...) { - QJniEnvironment env; if (clazz && methodId) { + QJniEnvironment env; va_list args; va_start(args, methodId); - QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, methodId, args)); + QJniObject res = getCleanJniObject(env->CallStaticObjectMethodV(clazz, methodId, args), env.jniEnv()); va_end(args); return res; } @@ -1171,7 +1191,7 @@ QJniObject QJniObject::getStaticObjectField(const char *className, if (!id) return QJniObject(); - return getCleanJniObject(env->GetStaticObjectField(clazz, id)); + return getCleanJniObject(env->GetStaticObjectField(clazz, id), env.jniEnv()); } /*! @@ -1194,7 +1214,7 @@ QJniObject QJniObject::getStaticObjectField(jclass clazz, const char *fieldName, if (!id) return QJniObject(); - return getCleanJniObject(env->GetStaticObjectField(clazz, id)); + return getCleanJniObject(env->GetStaticObjectField(clazz, id), env.jniEnv()); } /*! @@ -1233,12 +1253,11 @@ QJniObject QJniObject::getStaticObjectField(jclass clazz, const char *fieldName, */ QJniObject QJniObject::getObjectField(const char *fieldName, const char *signature) const { - QJniEnvironment env; - jfieldID id = getCachedFieldID(env.jniEnv(), fieldName, signature); + jfieldID id = getCachedFieldID(jniEnv(), fieldName, signature); if (!id) return QJniObject(); - return getCleanJniObject(env->GetObjectField(d->m_jobject, id)); + return getCleanJniObject(jniEnv()->GetObjectField(d->m_jobject, id), jniEnv()); } /*! @@ -1291,7 +1310,7 @@ QJniObject QJniObject::fromString(const QString &string) QJniEnvironment env; jstring stringRef = env->NewString(reinterpret_cast(string.constData()), string.length()); - QJniObject stringObject = getCleanJniObject(stringRef); + QJniObject stringObject = getCleanJniObject(stringRef, env.jniEnv()); stringObject.d->m_className = "java/lang/String"; return stringObject; } @@ -1316,10 +1335,9 @@ QString QJniObject::toString() const return QString(); QJniObject string = callObjectMethod("toString"); - QJniEnvironment env; - const int strLength = env->GetStringLength(string.object()); + const int strLength = string.jniEnv()->GetStringLength(string.object()); QString res(strLength, Qt::Uninitialized); - env->GetStringRegion(string.object(), 0, strLength, reinterpret_cast(res.data())); + string.jniEnv()->GetStringRegion(string.object(), 0, strLength, reinterpret_cast(res.data())); return res; } @@ -1377,13 +1395,17 @@ bool QJniObject::isValid() const QJniObject QJniObject::fromLocalRef(jobject lref) { QJniObject obj(lref); - QJniEnvironment()->DeleteLocalRef(lref); + obj.jniEnv()->DeleteLocalRef(lref); return obj; } bool QJniObject::isSameObject(jobject obj) const { - return QJniEnvironment()->IsSameObject(d->m_jobject, obj); + if (d->m_jobject == obj) + return true; + if (!d->m_jobject || !obj) + return false; + return jniEnv()->IsSameObject(d->m_jobject, obj); } bool QJniObject::isSameObject(const QJniObject &other) const @@ -1398,7 +1420,7 @@ void QJniObject::assign(jobject obj) d = QSharedPointer::create(); if (obj) { - QJniEnvironment env; + JNIEnv *env = d->jniEnv(); d->m_jobject = env->NewGlobalRef(obj); jclass objectClass = env->GetObjectClass(obj); d->m_jclass = static_cast(env->NewGlobalRef(objectClass)); diff --git a/src/corelib/kernel/qjniobject.h b/src/corelib/kernel/qjniobject.h index 88214a9311..1f6433d44e 100644 --- a/src/corelib/kernel/qjniobject.h +++ b/src/corelib/kernel/qjniobject.h @@ -18,24 +18,42 @@ class Q_CORE_EXPORT QJniObject { template struct LocalFrame { - QJniEnvironment env; + mutable JNIEnv *env; bool hasFrame = false; - ~LocalFrame() { + explicit LocalFrame(JNIEnv *env = nullptr) noexcept + : env(env) + { + } + ~LocalFrame() + { if (hasFrame) env->PopLocalFrame(nullptr); } template - auto newLocalRef(QJniObject &&object) { + auto newLocalRef(jobject object) + { if (!hasFrame) { - if (env->PushLocalFrame(sizeof...(Args)) < 0) + if (jniEnv()->PushLocalFrame(sizeof...(Args)) < 0) return T{}; // JVM is out of memory, avoid making matters worse hasFrame = true; } - return static_cast(env->NewLocalRef(object.template object())); + return static_cast(jniEnv()->NewLocalRef(object)); + } + template + auto newLocalRef(QJniObject &&object) + { + return newLocalRef(object.template object()); + } + JNIEnv *jniEnv() const + { + if (!env) + env = QJniEnvironment().jniEnv(); + return env; + } + bool checkAndClearExceptions() + { + return env ? QJniEnvironment::checkAndClearExceptions(env) : false; } - JNIEnv *jniEnv() const { return env.jniEnv(); } - bool checkAndClearExceptions() { return env.checkAndClearExceptions(); } - template auto convertToJni(T &&value); template @@ -103,7 +121,7 @@ public: > auto callMethod(const char *methodName, const char *signature, Args &&...args) const { - LocalFrame frame; + LocalFrame frame(jniEnv()); if constexpr (QtJniTypes::isObjectType()) { return frame.template convertFromJni(callObjectMethod(methodName, signature, frame.convertToJni(std::forward(args))...)); @@ -148,7 +166,7 @@ public: { QtJniTypes::assertObjectType(); constexpr auto signature = QtJniTypes::methodSignature(); - LocalFrame frame; + LocalFrame frame(jniEnv()); return frame.template convertFromJni(callObjectMethod(methodName, signature, frame.convertToJni(std::forward(args))...)); } @@ -212,7 +230,10 @@ public: { QJniEnvironment env; jclass clazz = QJniObject::loadClass(className, env.jniEnv()); - return callStaticMethod(clazz, methodName, std::forward(args)...); + const jmethodID id = clazz ? getMethodID(env.jniEnv(), clazz, methodName, + QtJniTypes::methodSignature().data(), true) + : 0; + return callStaticMethod(clazz, id, std::forward(args)...); } template auto getField(const char *fieldName) const { - LocalFrame frame; + LocalFrame frame(jniEnv()); if constexpr (QtJniTypes::isObjectType()) { return frame.template convertFromJni(getObjectField(fieldName)); } else { @@ -401,12 +422,11 @@ public: > void setField(const char *fieldName, T value) { - QJniEnvironment env; constexpr auto signature = QtJniTypes::fieldSignature(); - jfieldID id = getCachedFieldID(env.jniEnv(), fieldName, signature); + jfieldID id = getCachedFieldID(jniEnv(), fieldName, signature); if (id) { - setFieldForType(env.jniEnv(), object(), id, value); - env.checkAndClearExceptions(); + setFieldForType(jniEnv(), object(), id, value); + QJniEnvironment::checkAndClearExceptions(jniEnv()); } } @@ -417,11 +437,10 @@ public: > void setField(const char *fieldName, const char *signature, T value) { - QJniEnvironment env; - jfieldID id = getCachedFieldID(env.jniEnv(), fieldName, signature); + jfieldID id = getCachedFieldID(jniEnv(), fieldName, signature); if (id) { - setFieldForType(env.jniEnv(), object(), id, value); - env.checkAndClearExceptions(); + setFieldForType(jniEnv(), object(), id, value); + QJniEnvironment::checkAndClearExceptions(jniEnv()); } } @@ -524,6 +543,7 @@ public: protected: QJniObject(Qt::Initialization) {} + JNIEnv *jniEnv() const noexcept; private: static jclass loadClass(const QByteArray &className, JNIEnv *env); @@ -686,7 +706,7 @@ private: static constexpr void setFieldForType(JNIEnv *env, jobject obj, jfieldID id, T value) { - LocalFrame frame; + LocalFrame frame(env); if constexpr (sameTypeForJni) env->SetBooleanField(obj, id, static_cast(value)); else if constexpr (sameTypeForJni) @@ -713,7 +733,7 @@ private: static constexpr void setStaticFieldForType(JNIEnv *env, jclass clazz, jfieldID id, T value) { - LocalFrame frame; + LocalFrame frame(env); if constexpr (sameTypeForJni) env->SetStaticBooleanField(clazz, id, static_cast(value)); else if constexpr (sameTypeForJni) diff --git a/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp b/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp index 364b0c5d62..c39a6d0343 100644 --- a/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp +++ b/tests/auto/corelib/kernel/qjniobject/tst_qjniobject.cpp @@ -307,7 +307,7 @@ void tst_QJniObject::compareOperatorTests() QJniObject stringObject2 = QJniObject::fromString(str); QVERIFY(stringObject != stringObject2); - jstring jstrobj = 0; + jstring jstrobj = nullptr; QJniObject invalidStringObject; QVERIFY(invalidStringObject == jstrobj);