Android: lazy initialization for method id.

base/android/jni_android.h

 // Copyright (c) 2012 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.
 
 #ifndef BASE_ANDROID_JNI_ANDROID_H_
 #define BASE_ANDROID_JNI_ANDROID_H_
 
 #include <jni.h>
 #include <sys/types.h>
 
 #include "base/android/scoped_java_ref.h"
+#include "base/atomicops.h"
 #include "base/compiler_specific.h"
+#include "base/logging.h"

[joth, 2012/10/02 17:47:43]

rather than pull this spew into the header, can you put the method definition
into the .cc?
you can either put 4 clearly named wrapper methods around the lazy get method ID
version, or you could leave the single templated method declaration in the .h
and then make 4 explicit instantiations of it in the .cc.

[bulach, 2012/10/03 13:09:00]

see above: I split this into its own file, let me know your thoughts there.

 
 namespace base {
 namespace android {
 
 // Used to mark symbols to be exported in a shared library's symbol table.
 #define JNI_EXPORT __attribute__ ((visibility("default")))
 
 // Contains the registration method information for initializing JNI bindings.
 struct RegistrationMethod {
   const char* name;
@@ skipping 106 matching lines @@
 // Returns true if an exception is pending in the provided JNIEnv*.
 bool HasException(JNIEnv* env);
 
 // If an exception is pending in the provided JNIEnv*, this function clears it
 // and returns true.
 bool ClearException(JNIEnv* env);
 
 // This function will call CHECK() macro if there's any pending exception.
 void CheckException(JNIEnv* env);
 
+class LazyMethodID {

[joth, 2012/10/02 17:47:43]

needs class (function) doc
- when to use
- how to use
- (maybe general background on the perf tradeoff this is making for startup vs
runtime in the jni generator; we could put that detail into the .cc perhaps)

[bulach, 2012/10/03 13:09:00]

Done.

+ public:
+  enum MethodType {
+    METHODTYPE_STATIC,
+    METHODTYPE_NORMAL,
+  };
+
+  enum ExceptionCheck {
+    EXCEPTIONCHECK_YES,
+    EXCEPTIONCHECK_NO,
+  };

[joth, 2012/10/02 17:47:43]

enum duplication from the previous patch feels findof odd

[bulach, 2012/10/02 18:20:42]

sure, as soon as the other patch lands :)
fyi, I did this in a separate branch, I wasn't expecting them both to convert..

+
+  template<MethodType method_type, ExceptionCheck exception_check>
+  static void Get(JNIEnv* env,

[joth, 2012/10/02 17:47:43]

why is this in a class?

[bulach, 2012/10/03 13:09:00]

we already have a bunch of free functions here, and specially with the enums, it
seems slightly clearer to put them all together?

+                  jclass clazz,
+                  const char* method_name,
+                  const char* jni_signature,
+                  jmethodID* method_id) {

[joth, 2012/10/02 17:47:43]

method_id should be passed in & out as a AtomicWord. (method_id_storage?)

Return the method_id as a return value.

The caller should always go via this method to convert their opaque storage into
a useable method id.

[bulach, 2012/10/02 18:20:42]

sorry, not sure I understood..
the caller has "static jmethodID foo = 0;"
are you suggesting doing the cast at the caller site?
or creating an opaque union type of sorts for this?

[joth, 2012/10/02 18:30:39]

Neither! :-)

Calling code:
 
static AtomicWord g_method_id_Foo_barBaz = 0;  // <-- The declared storage must
be of type AtomicWord
void callFooBarBaz() {
  ... 
  jmethodID method_id = LazyMethod::Get<XX, YY>(env, clazz,
g_method_id_Foo_barBaz);
  env->CallMethod(method_id);
}


...

class LazyMethod {
  jmethodID Get(env, clazz, AtomicWord* method_storage_in_out) {
...
}

[bulach, 2012/10/03 13:09:00]

Done.

+    subtle::AtomicWord* atomic = reinterpret_cast<subtle::AtomicWord*>(
+        method_id);

[joth, 2012/10/02 17:47:43]

nit: could compile assert that sizeof(AtomicWord) >= sizeof(jmethodID)

[bulach, 2012/10/03 13:09:00]

Done.

+    subtle::AtomicWord value = base::subtle::Acquire_Load(atomic);
+    if (value)
+      return;

[joth, 2012/10/02 17:47:43]

need to document on the function that the caller is responsible to zero-init the
method id storage.

[bulach, 2012/10/03 13:09:00]

Done.

+    jmethodID id = method_type == METHODTYPE_STATIC ?

[joth, 2012/10/02 17:47:43]

comment that if we get a race in loading the value here, we're OK with the
operations below being duplicated on each thread as both will yield the same
result (we just experience a tiny bit of duplicated effort)

[bulach, 2012/10/03 13:09:00]

Done.

+        env->GetStaticMethodID(clazz, method_name, jni_signature) :

[joth, 2012/10/02 17:47:43]

as |env| is only need in uncommon case, we could call AttachCurrentThread rather
than require caller to always pass it. (although if in practice the caller has
to have one anyway in order to invoke the method looked up I guess this is fine
too)

[bulach, 2012/10/03 13:09:00]

so far, the caller seems to always have one.

+        env->GetMethodID(clazz, method_name, jni_signature);
+    if (exception_check == EXCEPTIONCHECK_YES) {
+      CHECK(id || base::android::ClearException(env)) <<
+        "Failed to find " <<
+        (method_type == METHODTYPE_STATIC ? "static " : "") <<
+        "method " << method_name << " " << jni_signature;
+    } else if (base::android::HasException(env)) {
+      env->ExceptionClear();

[joth, 2012/10/02 17:47:43]

can we call an existing GetMethodID blah class here?

[bulach, 2012/10/03 13:09:00]

see above.. I'd prefer to move towards this api and remove the old one.
thoughts?

+    }
+    base::subtle::Acquire_Store(

[joth, 2012/10/02 17:47:43]

This needs to be a Release_Store

[bulach, 2012/10/03 13:09:00]

Done.

+        atomic, reinterpret_cast<subtle::AtomicWord>(id));
+  }
+};
+
+
 }  // namespace android
 }  // namespace base
 
 #endif  // BASE_ANDROID_JNI_ANDROID_H_