Skip to content

perf(android): resolve a method's declared return class once per call site #125

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

Open
ammarahm-ed wants to merge 1 commit into from
Open

perf(android): resolve a method's declared return class once per call site #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 25 additions & 2 deletions NativeScript/ffi/jni/jsi/callbackhandlers/CallbackHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,27 @@ void CallbackHandlers::Init(JsRuntime &rt) {
MethodCache::Init();
}

static MetadataNode *ResolveDeclaredReturnNode(JEnv &jEnv, MetadataEntry *entry, jobject result) {
if (entry == nullptr) return nullptr;
if (!entry->returnClassResolved) {
entry->returnClassResolved = true;
const auto &returnType = entry->getReturnType();
if (returnType.size() > 2 && returnType[0] == 'L') {
const std::string declaredName = returnType.substr(1, returnType.size() - 2);
jclass declaredClass = jEnv.FindClass(declaredName);
if (declaredClass != nullptr) {
entry->returnClass = declaredClass;
entry->returnNode = MetadataNode::GetOrCreate(declaredName);
} else {
jEnv.ExceptionClear();
}
}
}
if (entry->returnClass == nullptr) return nullptr;
JniLocalRef runtimeClass(jEnv.GetObjectClass(result));
return jEnv.isSameObject(runtimeClass, entry->returnClass) ? entry->returnNode : nullptr;
}

JsValue CallbackHandlers::CallJavaMethod(JsRuntime &rt, const JsValue &caller, const string &className,
const string &methodName, MetadataEntry *entry,
bool isFromInterface, bool isStatic, bool isConstructorCall,
Expand Down Expand Up @@ -496,8 +517,10 @@ JsValue CallbackHandlers::CallJavaMethod(JsRuntime &rt, const JsValue &caller, c
returnValue = objectManager->GetJsObjectByJavaObject(javaObjectID);

if (js_util::is_null_or_undefined(returnValue)) {
returnValue = objectManager->CreateJSWrapper(javaObjectID, *returnType,
result);
MetadataNode *returnNode = ResolveDeclaredReturnNode(jEnv, entry, result);
returnValue = returnNode != nullptr
? objectManager->CreateJSWrapper(javaObjectID, returnNode, entry->returnClass)
: objectManager->CreateJSWrapper(javaObjectID, *returnType, result);
}
}

Expand Down
8 changes: 8 additions & 0 deletions NativeScript/ffi/jni/jsi/metadata/MetadataEntry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include "MetadataMethodInfo.h"
#include "MetadataFieldInfo.h"

class MetadataNode;

namespace tns {
enum class NodeType {
Package,
Expand Down Expand Up @@ -49,6 +51,9 @@ namespace tns {
memberId = other.memberId;
clazz = other.clazz;
parsedSig = other.parsedSig;
returnClass = other.returnClass;
returnNode = other.returnNode;
returnClassResolved = other.returnClassResolved;
mi = other.mi;
fi = other.fi;
sfi = other.sfi;
Expand Down Expand Up @@ -91,6 +96,9 @@ namespace tns {
void *memberId;
jclass clazz;
std::vector<std::string> parsedSig;
jclass returnClass = nullptr;
::MetadataNode *returnNode = nullptr;
bool returnClassResolved = false;

MethodInfo mi;
FieldInfo *fi;
Expand Down
26 changes: 15 additions & 11 deletions NativeScript/ffi/jni/jsi/objectmanager/ObjectManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,20 +631,24 @@ JsValue ObjectManager::CreateJSWrapperHelper(jint javaObjectID, const std::strin
auto className = (clazz != nullptr) ? GetClassName(clazz) : typeName;

auto node = MetadataNode::GetOrCreate(className);
// Reuse the class we already resolved via GetObjectClass on the instance
// path instead of re-resolving it with a JNI FindClass. The class is only
// stored on JSInstanceInfo::ObjectClazz, which nothing on this path reads,
// so a fresh FindClass is pure overhead; only fall back to it for the
// typeName-only overload where no instance class was available.
jclass linkClazz = clazz;
if (linkClazz == nullptr) {
JEnv jenv;
linkClazz = jenv.FindClass(className);
}
return CreateJSWrapper(javaObjectID, node, linkClazz);
}

JsValue ObjectManager::CreateJSWrapper(jint javaObjectID, MetadataNode *node, jclass clazz) {
JsValue proxy = js_util::undefined();
JsValue jsWrapper = node->CreateJSWrapper(*m_rt, this);
if (jsWrapper.isObject()) {
// Reuse the class we already resolved via GetObjectClass on the instance
// path instead of re-resolving it with a JNI FindClass. The class is only
// stored on JSInstanceInfo::ObjectClazz, which nothing on this path reads,
// so a fresh FindClass is pure overhead; only fall back to it for the
// typeName-only overload where no instance class was available.
jclass linkClazz = clazz;
if (linkClazz == nullptr) {
JEnv jenv;
linkClazz = jenv.FindClass(className);
}
Link(jsWrapper, javaObjectID, linkClazz, node);
Link(jsWrapper, javaObjectID, clazz, node);
if (node->isArray()) {
jsWrapper.asObject(*m_rt).setProperty(*m_rt, "__is__javaArray", true);
}
Expand Down
2 changes: 2 additions & 0 deletions NativeScript/ffi/jni/jsi/objectmanager/ObjectManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ namespace tns {

JsValue CreateJSWrapper(jint javaObjectID, const std::string &typeName, jobject instance);

JsValue CreateJSWrapper(jint javaObjectID, MetadataNode *node, jclass clazz);

JsValue GetOrCreateProxy(jint javaObjectID, const JsValue &instance);

JsValue GetOrCreateProxyWeak(jint javaObjectID, const JsValue &instance);
Expand Down
27 changes: 25 additions & 2 deletions NativeScript/ffi/jni/napi/callbackhandlers/CallbackHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,27 @@ void CallbackHandlers::Init(napi_env env) {
MethodCache::Init();
}

static MetadataNode *ResolveDeclaredReturnNode(JEnv &jEnv, MetadataEntry *entry, jobject result) {
if (entry == nullptr) return nullptr;
if (!entry->returnClassResolved) {
entry->returnClassResolved = true;
const auto &returnType = entry->getReturnType();
if (returnType.size() > 2 && returnType[0] == 'L') {
const std::string declaredName = returnType.substr(1, returnType.size() - 2);
jclass declaredClass = jEnv.FindClass(declaredName);
if (declaredClass != nullptr) {
entry->returnClass = declaredClass;
entry->returnNode = MetadataNode::GetOrCreate(declaredName);
} else {
jEnv.ExceptionClear();
}
}
}
if (entry->returnClass == nullptr) return nullptr;
JniLocalRef runtimeClass(jEnv.GetObjectClass(result));
return jEnv.isSameObject(runtimeClass, entry->returnClass) ? entry->returnNode : nullptr;
}

napi_value CallbackHandlers::CallJavaMethod(napi_env env, napi_value caller, const string &className,
const string &methodName, MetadataEntry *entry,
bool isFromInterface, bool isStatic, napi_callback_info info, size_t argc, napi_value* argv,
Expand Down Expand Up @@ -500,8 +521,10 @@ napi_value CallbackHandlers::CallJavaMethod(napi_env env, napi_value caller, con
returnValue = objectManager->GetJsObjectByJavaObject(javaObjectID);

if (napi_util::is_null_or_undefined(env, returnValue)) {
returnValue = objectManager->CreateJSWrapper(javaObjectID, *returnType,
result);
MetadataNode *returnNode = ResolveDeclaredReturnNode(jEnv, entry, result);
returnValue = returnNode != nullptr
? objectManager->CreateJSWrapper(javaObjectID, returnNode, entry->returnClass)
: objectManager->CreateJSWrapper(javaObjectID, *returnType, result);
}
}

Expand Down
8 changes: 8 additions & 0 deletions NativeScript/ffi/jni/napi/metadata/MetadataEntry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include "MetadataMethodInfo.h"
#include "MetadataFieldInfo.h"

class MetadataNode;

namespace tns {
enum class NodeType {
Package,
Expand Down Expand Up @@ -49,6 +51,9 @@ namespace tns {
memberId = other.memberId;
clazz = other.clazz;
parsedSig = other.parsedSig;
returnClass = other.returnClass;
returnNode = other.returnNode;
returnClassResolved = other.returnClassResolved;
mi = other.mi;
fi = other.fi;
sfi = other.sfi;
Expand Down Expand Up @@ -91,6 +96,9 @@ namespace tns {
void *memberId;
jclass clazz;
std::vector<std::string> parsedSig;
jclass returnClass = nullptr;
::MetadataNode *returnNode = nullptr;
bool returnClassResolved = false;

MethodInfo mi;
FieldInfo *fi;
Expand Down
28 changes: 16 additions & 12 deletions NativeScript/ffi/jni/napi/objectmanager/ObjectManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,24 +818,28 @@ ObjectManager::CreateJSWrapper(jint javaObjectID, const std::string &typeName, j

napi_value
ObjectManager::CreateJSWrapperHelper(jint javaObjectID, const std::string &typeName, jclass clazz) {
napi_status status;
auto className = (clazz != nullptr) ? GetClassName(clazz) : typeName;

auto node = MetadataNode::GetOrCreate(className);
// Reuse the class we already resolved via GetObjectClass on the instance
// path instead of re-resolving it with a JNI FindClass. The class is only
// stored on JSInstanceInfo::ObjectClazz, which nothing on this path reads,
// so a fresh FindClass is pure overhead; only fall back to it for the
// typeName-only overload where no instance class was available.
jclass linkClazz = clazz;
if (linkClazz == nullptr) {
JEnv jenv;
linkClazz = jenv.FindClass(className);
}
return CreateJSWrapper(javaObjectID, node, linkClazz);
}

napi_value ObjectManager::CreateJSWrapper(jint javaObjectID, MetadataNode *node, jclass clazz) {
napi_status status;
napi_value proxy = nullptr;
napi_value jsWrapper = node->CreateJSWrapper(m_env, this);
if (jsWrapper != nullptr) {
// Reuse the class we already resolved via GetObjectClass on the instance
// path instead of re-resolving it with a JNI FindClass. The class is only
// stored on JSInstanceInfo::ObjectClazz, which nothing on this path reads,
// so a fresh FindClass is pure overhead; only fall back to it for the
// typeName-only overload where no instance class was available.
jclass linkClazz = clazz;
if (linkClazz == nullptr) {
JEnv jenv;
linkClazz = jenv.FindClass(className);
}
Link(jsWrapper, javaObjectID, linkClazz, node);
Link(jsWrapper, javaObjectID, clazz, node);
if (node->isArray()) {
NAPI_GUARD(napi_set_named_property(m_env, jsWrapper, "__is__javaArray",
napi_util::get_true(m_env))) {}
Expand Down
2 changes: 2 additions & 0 deletions NativeScript/ffi/jni/napi/objectmanager/ObjectManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ namespace tns {
napi_value
CreateJSWrapper(jint javaObjectID, const std::string &typeName, jobject instance);

napi_value CreateJSWrapper(jint javaObjectID, MetadataNode *node, jclass clazz);

napi_value GetOrCreateProxy(jint javaObjectID, napi_value instance);

napi_value GetOrCreateProxyWeak(jint javaObjectID, napi_value instance);
Expand Down
Loading