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

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

The Java Bridge makes repeated use of numerous JNI method IDs. We wish to avoid
repeatedly looking up these IDS, so we cache them.

Currently we use Singleton objects to achieve this. However, this seems
inappropriate for several reasons ...
- Requires excessive boilerplate
- Thread safety not required
- Exit cleanup not required

We can avoid using Singleton, without introducing static initializers, by
simply using a static local.

See http://codereview.chromium.org/8659007 for background.

TBR=avi
BUG=105547
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112975

[joth, 2011-12-01 12:55:25]

Fab, thanks. I think this is a much better pattern (for single threaded users,
at least)

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

(Note the comments below aren't in the right order)

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.cc#new...
base/android/jni_android.cc:23: jint ret = g_jvm->AttachCurrentThread(&env,
NULL);
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().

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.cc#new...
base/android/jni_android.cc:54: ScopedJavaLocalRef<jclass> clazz(env,
env->FindClass(class_name));
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?

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.h#newc...
base/android/jni_android.h:36: jmethodID GetMethodIDFromClassName(JNIEnv* env,
I'm taking issue about the design now. Here's the point I'm noting here:

- While the |env| argument is necessary, it's guaranteed to be a constant value
for the caller. In fact, it adds burdens to the caller without any benefit. I'd
prefer this layer to hide this fact.
- You leave value caching to the client. But that will inevitably result in
multiple near implementations in each components calling it. Assuming the return
values are guaranteed to be constant (?), then it'd be preferable to have the
caching transparent here.

http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_bound_object.cc (left):

http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bound_object.cc:487: return
Singleton<ObjectGetClassID>::get();
If you had used LazyInstance in the first place, you wouldn't have had atexit()
registration. Ironically by using a static local variable, you add an implicit
atexit call.

http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_bound_object.cc (right):

http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bound_object.cc:485: static jmethodID id
= GetMethodIDFromClassName(AttachCurrentThread(),
I'm really not a fan of static locals. Especially that your copy pasted code
could have been simplified even more than what you just wrote:

template<const char X, const char Y, const char Z>
class ObjectGetXXID : MethodID {
 public:
  ObjectGetXXID()
      : MethodID(AttachCurrentThread(), X, Y, Z) {
  }
 private:
  DISALLOW_COPY_AND_ASSIGN(ObjectGetXXID);
}

const char kJavaLangObject[] = "java/lang/Object";
const char kGetClass[] = "getClass";
const char kQJavaLangClass[] = "()Ljava/lang/Class;";
const char kGetMethods[] = "getMethods";
const char kQBJavaLangReflectMethod[] = "()[Ljava/lang/reflect/Method;";

const char kJavaLangReflectMethod[] = "java/lang/reflect/Method";
const char kGetParameterTypes[] = "getParameterTypes";
const char kQBJavaLangClass[] = "()[Ljava/lang/Class;";

typedef ObjectGetXXId<kJavaLangObject, kGetClass, kQJavaLangClass>
ObjectGetClassID;
typedef ObjectGetXXId<kJavaLangObject, kGetMethods, kQBJavaLangClass>
ObjectGetClassID;
typedef ObjectGetXXId<kJavaLangReflectMethod, kGetParameterTypes,
kQBJavaLangClass> ObjectGetClassID;

Note the implementation above is incomplete, it also needs the LazyInstance
implementation but in any case, I think it's the wrong layer to do caching so it
was just to make a point that the copy pasted classes could have been simplified
already.

[Steve Block, 2011-12-01 17:48:42]

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.cc#new...
base/android/jni_android.cc:23: jint ret = g_jvm->AttachCurrentThread(&env,
NULL);
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.

http://codereview.chromium.org/8769005/diff/1/base/android/jni_android.cc#new...
base/android/jni_android.cc:54: ScopedJavaLocalRef<jclass> clazz(env,
env->FindClass(class_name));
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.

[joth, 2011-12-02 11:39:02]

http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
File content/browser/renderer_host/java/java_bound_object.cc (left):

http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
content/browser/renderer_host/java/java_bound_object.cc:487: return
Singleton<ObjectGetClassID>::get();
On 2011/12/01 14:36:03, Marc-Antoine Ruel wrote:
> If you had used LazyInstance in the first place, you wouldn't have had
atexit()
> registration. Ironically by using a static local variable, you add an implicit
> atexit call.

I don't quite follow this bit-
- both Singleton<> and LazyInstance<> register with AtExit to call the
destructor, unless you use the leaky traits variants of the same. The choice
Singleton vs LazyInstnace doesn't seem to matter?
- A jmethodID is typedef for a struct pointer. A pointer is POD so why would
declaring a static pointer cause an implicit atexit call?

related observations:
=> thakis is making great headway in removing static initializers and atexit
calls, and a good chunk of his fixes are just replacing local static class
instances with a static reference (e.g.
http://codereview.chromium.org/8480033/diff/27/chrome/browser/chromeos/cros/n...)

=> Should CR_DEFINE_STATIC_LOCAL even exist if this is not the correct solution?
http://codesearch.google.com/#OAMlx_jo-ck/src/base/basictypes.h&exact_package...
(oh heck, I hope this isn't a static pointer vs reference difference?)

[M-A Ruel, 2011-12-02 12:40:31]

On 2011/12/02 11:39:02, joth wrote:
>
http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
> File content/browser/renderer_host/java/java_bound_object.cc (left):
> 
>
http://codereview.chromium.org/8769005/diff/1/content/browser/renderer_host/j...
> content/browser/renderer_host/java/java_bound_object.cc:487: return
> Singleton<ObjectGetClassID>::get();
> On 2011/12/01 14:36:03, Marc-Antoine Ruel wrote:
> > If you had used LazyInstance in the first place, you wouldn't have had
> atexit()
> > registration. Ironically by using a static local variable, you add an
implicit
> > atexit call.

Yeah, I'm probably wrong on this. You should probably use the Leaky one in fact.

> (oh heck, I hope this isn't a static pointer vs reference difference?)
If it's a POD, it should be fine.


lgtm

[M-A Ruel, 2011-12-02 12:40:57]

http://codereview.chromium.org/8769005/diff/5001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/8769005/diff/5001/base/android/jni_android.cc#...
base/android/jni_android.cc:65: DCHECK_EQ(std::string::npos,
std::string(class_name).find('!'));
Can this code ever be called from unstruted sources? E.g. is it possible to have
user supplied (let's say from javascript) data being used as input to this
function?

[Steve Block, 2011-12-02 12:49:10]

http://codereview.chromium.org/8769005/diff/5001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/8769005/diff/5001/base/android/jni_android.cc#...
base/android/jni_android.cc:65: DCHECK_EQ(std::string::npos,
std::string(class_name).find('!'));
No. Currently it's only used with constants. I'll add a comment to the header to
make this clear.

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

LGTM with a couple suggestions that could be done later if preferred ...

http://codereview.chromium.org/8769005/diff/5003/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/8769005/diff/5003/base/android/jni_android.cc#...
base/android/jni_android.cc:20: base::LazyInstance<MethodIDMap> g_method_id_map
= LAZY_INSTANCE_INITIALIZER;
fwiw, I think we're happy to use LeakyLazyInstanceTraits for this sort of thing.

http://codereview.chromium.org/8769005/diff/5003/base/android/jni_android.cc#...
base/android/jni_android.cc:72: key += jni_signature;
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)

http://codereview.chromium.org/8769005/diff/5003/base/android/jni_android.cc#...
base/android/jni_android.cc:81: }
looks sufficiently fun to benefit from having some tests.

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

http://codereview.chromium.org/8769005/diff/5003/base/android/jni_android.cc#...
base/android/jni_android.cc:103: if (iter == map->end()) {
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;

[joth, 2011-12-02 17:35:36]

lg from my side...

http://codereview.chromium.org/8769005/diff/5003/content/browser/renderer_hos...
File content/browser/renderer_host/java/java_bound_object.cc (right):

http://codereview.chromium.org/8769005/diff/5003/content/browser/renderer_hos...
content/browser/renderer_host/java/java_bound_object.cc:570:
"()Ljava/lang/Class;"))));
No problem with named constants, but fwiw these literals are also fine; they
can't go out of scope in practice. (TRACE_EVENT already relies on this, for one)

http://codereview.chromium.org/8769005/diff/5004/base/android/get_method_id_f...
File base/android/get_method_id_from_class_name_unittest.cc (right):

http://codereview.chromium.org/8769005/diff/5004/base/android/get_method_id_f...
base/android/get_method_id_from_class_name_unittest.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
IMO calling it jni_android_unittest.cc is consistent and fine, even if it
doesn't [yet] contain tests for everything in that file.
(reduces the barrier to future maintainer increasing test coverage)

http://codereview.chromium.org/8769005/diff/5004/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/8769005/diff/5004/base/android/jni_android.h#n...
base/android/jni_android.h:37: // entire duration for which this function is in
use. In practice, this means
nit: "entire duration for which this function is in use" is a bit ambiguous.
How about "The strings passed to this function are held in the cache and MUST
remain valid after the function returns, and beyond the duration of all futures
calls to this function, across all threads. In practice..."

[Steve Block, 2011-12-02 18:05:18]

Thanks for the reviews, will land on Monday if no further comments.

[commit-bot: I haz the power, 2011-12-05 10:38:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/8769005/5005

[commit-bot: I haz the power, 2011-12-05 10:38:13]

Presubmit check for 8769005-5005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/java/java_method.cc,content/browser/renderer_host/java/java_bound_object.cc

Presubmit checks took 1.5s to calculate.

[Steve Block, 2011-12-05 10:44:31]

+avi for content/browser/renderer_host

[M-A Ruel, 2011-12-05 11:51:20]

On 2011/12/05 10:44:31, Steve Block wrote:
> +avi for content/browser/renderer_host

Setting as TBR, it's been a long enough review.

[commit-bot: I haz the power, 2011-12-05 11:51:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/8769005/5005

[commit-bot: I haz the power, 2011-12-05 14:42:13]

Change committed as 112975