From fedf76100047b15eab625825f503d0564db8a1af Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Tue, 12 Nov 2024 13:10:42 +0100 Subject: [PATCH] JNI: Optimize string handling A lot of code passes strings through JNI calls, and we always ended up constructing a temporary QJniObject just to convert it to a QString, or to get the jstring out of it. This is expensive as QJniObject allocates a private object and maintains a reference count. For code paths that have a jstring, or only want a jstring, and already have a JNIEnv pointer, we can shortcut this and use JNI APIs directly rather than going through the QJniObject detour. This was already done when accessing elements of a QJniArray. Add internal helper methods to the QtJniTypes::Details namespace that convert between jstring and QString directly. Use that where we can, including the QJniObject::to/fromString implementations. Since the new helper for converting from a QString already returns a local reference, split the LocalFrame::newLocalRef into a helper that pushes the local frame, and use that to explicitly do so in the case of QString conversion. This also removes the last use of the QJniObject overload of LocalFrame::newLocalRef, so remove that. Change-Id: Ib3a261efd0dcca86a1a99c2a054d50e932c3fe71 Reviewed-by: Soheil Armin Reviewed-by: Assam Boudjelthia --- src/corelib/kernel/qjniarray.h | 6 +++--- src/corelib/kernel/qjniobject.cpp | 8 ++------ src/corelib/kernel/qjniobject.h | 22 ++++++++++++---------- src/corelib/kernel/qjnitypes.h | 2 +- src/corelib/kernel/qjnitypes_impl.h | 18 +++++++++++++++++- 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/corelib/kernel/qjniarray.h b/src/corelib/kernel/qjniarray.h index 8bf313db735..cd85ae48043 100644 --- a/src/corelib/kernel/qjniarray.h +++ b/src/corelib/kernel/qjniarray.h @@ -735,9 +735,7 @@ public: } else if constexpr (std::is_same_v) { jstring string = static_cast(env->GetObjectArrayElement(arrayObject(), i)); if (string) { - const auto length = env->GetStringLength(string); - QString res(length, Qt::Uninitialized); - env->GetStringRegion(string, 0, length, reinterpret_cast(res.data())); + QString res = QtJniTypes::Detail::toQString(string, env); env->DeleteLocalRef(string); return res; } else { @@ -817,6 +815,8 @@ public: for (auto element : *this) { if constexpr (std::is_same_v) container.emplace_back(element); + else if constexpr (std::is_same_v) + container.emplace_back(QtJniTypes::Detail::toQString(element, env)); else container.emplace_back(QJniObject(element).toString()); } diff --git a/src/corelib/kernel/qjniobject.cpp b/src/corelib/kernel/qjniobject.cpp index 2a3c365a0e7..b780d0e55b7 100644 --- a/src/corelib/kernel/qjniobject.cpp +++ b/src/corelib/kernel/qjniobject.cpp @@ -1325,8 +1325,7 @@ QJniObject QJniObject::getObjectField(const char *fieldName, const char *signatu QJniObject QJniObject::fromString(const QString &string) { QJniEnvironment env; - jstring stringRef = env->NewString(reinterpret_cast(string.constData()), - string.length()); + jstring stringRef = QtJniTypes::Detail::fromQString(string, env.jniEnv()); QJniObject stringObject = getCleanJniObject(stringRef, env.jniEnv()); stringObject.d->m_className = "java/lang/String"; return stringObject; @@ -1352,10 +1351,7 @@ QString QJniObject::toString() const return QString(); QJniObject string = callObjectMethod("toString"); - const int strLength = string.jniEnv()->GetStringLength(string.object()); - QString res(strLength, Qt::Uninitialized); - string.jniEnv()->GetStringRegion(string.object(), 0, strLength, reinterpret_cast(res.data())); - return res; + return QtJniTypes::Detail::toQString(string.object(), string.jniEnv()); } /*! diff --git a/src/corelib/kernel/qjniobject.h b/src/corelib/kernel/qjniobject.h index 76f1c0787ec..e59773f8a77 100644 --- a/src/corelib/kernel/qjniobject.h +++ b/src/corelib/kernel/qjniobject.h @@ -31,21 +31,21 @@ class Q_CORE_EXPORT QJniObject if (hasFrame) env->PopLocalFrame(nullptr); } + bool ensureFrame() + { + if (!hasFrame) + hasFrame = jniEnv()->PushLocalFrame(sizeof...(Args)) == 0; + return hasFrame; + } template auto newLocalRef(jobject object) { - if (!hasFrame) { - if (jniEnv()->PushLocalFrame(sizeof...(Args)) < 0) - return T{}; // JVM is out of memory, avoid making matters worse - hasFrame = true; + if (!ensureFrame()) { + // if the JVM is out of memory, avoid making matters worse + return T{}; } return static_cast(jniEnv()->NewLocalRef(object)); } - template - auto newLocalRef(const QJniObject &object) - { - return newLocalRef(object.template object()); - } JNIEnv *jniEnv() const { if (!env) @@ -870,7 +870,9 @@ auto QJniObject::LocalFrame::convertToJni(T &&value) { using Type = q20::remove_cvref_t; if constexpr (std::is_same_v) { - return newLocalRef(QJniObject::fromString(value)); + if (ensureFrame()) // fromQString already returns a local reference + return QtJniTypes::Detail::fromQString(value, jniEnv()); + return jstring{}; } else if constexpr (QtJniTypes::IsJniArray::value) { return value.arrayObject(); } else if constexpr (QtJniTypes::detail::isCompatibleSourceContainer) { diff --git a/src/corelib/kernel/qjnitypes.h b/src/corelib/kernel/qjnitypes.h index d717c0c0d25..37b0bde2bd2 100644 --- a/src/corelib/kernel/qjnitypes.h +++ b/src/corelib/kernel/qjnitypes.h @@ -163,7 +163,7 @@ struct JNITypeForArgImpl static QString fromVarArg(Type t) { - return QJniObject(t).toString(); + return QtJniTypes::Detail::toQString(t, QJniEnvironment::getJniEnv()); } }; diff --git a/src/corelib/kernel/qjnitypes_impl.h b/src/corelib/kernel/qjnitypes_impl.h index d9635090238..6324ace17de 100644 --- a/src/corelib/kernel/qjnitypes_impl.h +++ b/src/corelib/kernel/qjnitypes_impl.h @@ -4,7 +4,7 @@ #ifndef QJNITYPES_IMPL_H #define QJNITYPES_IMPL_H -#include +#include #include #if defined(Q_QDOC) || defined(Q_OS_ANDROID) @@ -15,6 +15,22 @@ QT_BEGIN_NAMESPACE namespace QtJniTypes { +namespace Detail +{ +static inline jstring fromQString(const QString &string, JNIEnv *env) +{ + return env->NewString(reinterpret_cast(string.constData()), string.length()); +} + +static inline QString toQString(jstring string, JNIEnv *env) +{ + const jsize length = env->GetStringLength(string); + QString res(length, Qt::Uninitialized); + env->GetStringRegion(string, 0, length, reinterpret_cast(res.data())); + return res; +} +} // namespace Detail + // a constexpr type for string literals of any character width, aware of the length // of the string. template