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 <map>
+
 #include "base/android/scoped_java_ref.h"
+#include "base/atomicops.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/threading/platform_thread.h"
 
 namespace {
 JavaVM* g_jvm = 0;
 jobject g_application_context = NULL;
+
+typedef std::map<std::string, jmethodID> MethodIDMap;
+base::LazyInstance<MethodIDMap> g_method_id_map = LAZY_INSTANCE_INITIALIZER;

[joth, 2011/12/02 14:10:13]

fwiw, I think we're happy to use LeakyLazyInstanceTraits for this sort of thing.

+const base::subtle::AtomicWord kUnlocked = 0;
+const base::subtle::AtomicWord kLocked = 1;
+base::subtle::AtomicWord g_method_id_map_lock = kUnlocked;
 }
 
 namespace base {
 namespace android {
 
 JNIEnv* AttachCurrentThread() {
   if (!g_jvm)
     return NULL;
 
   JNIEnv* env = NULL;
@@ skipping 17 matching lines @@
 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) {
+  DCHECK_EQ(std::string::npos, std::string(class_name).find('!'));
+  DCHECK_EQ(std::string::npos, std::string(method).find('!'));
+  DCHECK_EQ(std::string::npos, std::string(jni_signature).find('!'));
+  std::string key(class_name);
+  key += '!';
+  key += method;
+  key += '!';
+  key += jni_signature;

[joth, 2011/12/02 14:10:13]

shame we have to do this everytime we lookup a symbol. Seems it may outweigh the
savings of caching... (which as mentioned the VM could be doing anyway)... we
need a micro-benchmark to know for sure.

As an alternative:

struct MethodPath {
  const char* class;
  const char* method;
  const char* sig;
  bool operator<(const MethodPath& other) const {
    int r = strcmp(class, other.class);
    if (r < 0) return true;
    if (r > 0) return false;

    r = strcmp(method, other.method);
    if (r < 0) return true;
    if (r > 0) return false;

    retrun strcmp(sig, other.sig) < 0;
  }
}
 typedef std::map<MethodPath, jmethodID> MethodIDMap;

then mandate the API is only used with literals, and no deep copies are needed
at all.

Anyway, can easily do this in a followup if needed. (modulo, the need to change
API contract viz. passing literals only. Although, we can avoid that if needed
by having a companion std::set<string> to efficiently pool all the deep copies)

+
+  MethodIDMap* map = g_method_id_map.Pointer();
+  bool found = false;
+
+  while (base::subtle::Acquire_CompareAndSwap(&g_method_id_map_lock,
+                                              kUnlocked,
+                                              kLocked) != kUnlocked) {
+    base::PlatformThread::YieldCurrentThread();
+  }

[joth, 2011/12/02 14:10:13]

looks sufficiently fun to benefit from having some tests.

JNIEnv is moderately mock-friendly (at least, given this is an android-only
file)

+  MethodIDMap::const_iterator iter = map->find(key);
+  if (iter != map->end()) {
+    found = true;
+  }
+  base::subtle::Release_Store(&g_method_id_map_lock, kUnlocked);
+
+  // Addition to the map does not invalidate this iterator.
+  if (found) {
+    return iter->second;
+  }
+
   ScopedJavaLocalRef<jclass> clazz(env, env->FindClass(class_name));
-  id_ = GetMethodID(env, clazz.obj(), method, jni_signature);
+  jmethodID id = GetMethodID(env, clazz.obj(), method, jni_signature);
+
+  while (base::subtle::Acquire_CompareAndSwap(&g_method_id_map_lock,
+                                              kUnlocked,
+                                              kLocked) != kUnlocked) {
+    base::PlatformThread::YieldCurrentThread();
+  }
+  // Another thread may have populated the map already.
+  iter = map->find(key);
+  if (iter == map->end()) {

[joth, 2011/12/02 14:10:13]

nit: rather than walk the tree twice here, you can do:

std::pair<MethodIDMap::const_iterator, bool> result =
    map->insert(std::make_pair(key, id));
DCHECK_EQ(id, result.first->second) << "did add=" << result.second;

+    map->insert(std::make_pair(key, id));
+  } else {
+    DCHECK_EQ(id, iter->second);
+  }
+  base::subtle::Release_Store(&g_method_id_map_lock, kUnlocked);
+
+  return id;
 }
 
 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