Don't use Singleton to cache JNI method IDs in Java Bridge

base/android/jni_android.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/android/jni_android.h"
 
 #include "base/android/scoped_java_ref.h"
 #include "base/logging.h"
 
 namespace {
 JavaVM* g_jvm = 0;
 jobject g_application_context = NULL;
 }
 
 namespace base {
 namespace android {
 
 JNIEnv* AttachCurrentThread() {
   if (!g_jvm)
     return NULL;
 
   JNIEnv* env = NULL;
   jint ret = g_jvm->AttachCurrentThread(&env, NULL);

[M-A Ruel, 2011/12/01 14:36:03]

I feel like this should be TLS cached. Isn't this call potentially expensive? I
don't know if it is refcounted correctly but I hope not because the number of
calls to AttachCurrentThread() != number of calls to DetachCurrentThread().

[Steve Block, 2011/12/01 17:48:47]

AttachCurrentThread() is intended to be used as a way to get the JNIEnv for an
already-attached thread, so there's no need to balance calls with
DetachCurrentThread(). Also, Dalvik caches the JNIEnvs in TLS to make the lookup
fast.

   DCHECK_EQ(ret, JNI_OK);
   return env;
 }
 
 void DetachFromVM() {
   // Ignore the return value, if the thread is not attached, DetachCurrentThread
   // will fail. But it is ok as the native thread may never be attached.
   if (g_jvm)
     g_jvm->DetachCurrentThread();
 }
 
 void InitVM(JavaVM* vm) {
   DCHECK(!g_jvm);
   g_jvm = vm;
 }
 
 void InitApplicationContext(jobject context) {
   DCHECK(!g_application_context);
   g_application_context = context;
 }
 
 jobject GetApplicationContext() {
   DCHECK(g_application_context);
   return g_application_context;
 }
 
-MethodID::MethodID(JNIEnv* env, const char* class_name, const char* method,
-                   const char* jni_signature) {
+jmethodID GetMethodIDFromClassName(JNIEnv* env,
+                                   const char* class_name,
+                                   const char* method,
+                                   const char* jni_signature) {
   ScopedJavaLocalRef<jclass> clazz(env, env->FindClass(class_name));

[M-A Ruel, 2011/12/01 14:36:03]

If you implement caching at this layer, you'll have a single TLS LazyInstance
blocked with a SWMR lock. I've sent a thread to chromium-dev since I couldn't
find an implementation in our code (!)

Then you just do a lookup. I'd do something like:
std::string buffer(class_name):
buffer += "\x01";
buffer += method;
buffer += "\x01";
buffer += jni_signature;

Where "\x01" would be any invalid java symbol. Then you just need a simple
binary tree. If you expect the number of items to be low, use a std::vector and
use a binary_search, it'll be faster.

What do you think?

[Steve Block, 2011/12/01 17:48:47]

There's no need to cache JNIEnv (see above) and method IDs are constant and can
be shared across threads. So we can implement caching at this level with a
single threadsafe map.

-  id_ = GetMethodID(env, clazz.obj(), method, jni_signature);
+  return GetMethodID(env, clazz.obj(), method, jni_signature);
 }
 
 jmethodID GetMethodID(JNIEnv* env,
                       jclass clazz,
                       const char* const method,
                       const char* const jni_signature) {
   jmethodID id = env->GetMethodID(clazz, method, jni_signature);
   DCHECK(id) << method;
   CheckException(env);
   return id;
@@ skipping 22 matching lines @@
 bool CheckException(JNIEnv* env) {
   if (env->ExceptionCheck() == JNI_FALSE)
     return false;
   env->ExceptionDescribe();
   env->ExceptionClear();
   return true;
 }
 
 }  // namespace android
 }  // namespace base