Android: lazy initialization for method id.

Android: lazy initialization for method id.

Rather than requiring early registration for all method id, we can initialize
them lazily as required.
This solves the problem of building against SDK X but running against X - 1.
Also adds a microbenchmark to ensure there are no considerable regressions.
Results are a bit variable, but it hovers over:
[ERROR:jni_android_unittest.cc(125)] JNI LazyMethodIDCall (us) 1983
[ERROR:jni_android_unittest.cc(127)] JNI MethodIDCall (us) 1862

BUG=152987
TEST=JNIAndroidMicrobenchmark.MethodId
TBR=akalin

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162186

[bulach, 2012-10-02 14:37:20]

hi!

Following the discussions on:
http://code.google.com/p/chromium/issues/detail?id=152987

this patch adds the "lazy initialization" for the jmethodid, with a
microbenchmark that seems positive..

this is not yet really tidy up, but I'd appreciate comments in the following
areas:
- LazyMethodID class and its implementation: it does a atomic read / write for
the method id.. the worst case scenario is two threads updating it at the same
time, but should be fine since the value will be the same.

- does the microbenchmark test suite seem valid?

I'll clean this up later if we agree that this is conceptually sound (fwiw, I
tested it downstream, didn't find any issues..).

[Sami, 2012-10-02 15:14:04]

Some comments inline. Mind posting your micro benchmark scores?

I remember some part of the JNI name resolution walking up the Java callstack to
find the classpath, which caused some classes to not be found if we were on the
"wrong" thread. Here we already have the jclass reference, so this probably
isn't an issue here.

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

http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h#new...
base/android/jni_android.h:149: EXCEPTIONCHECK_YES,
Super-nit: NO=0, YES=1 would seem more logical.

http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h#new...
base/android/jni_android.h:159: subtle::AtomicWord* atomic =
reinterpret_cast<subtle::AtomicWord*>(
I'm not sure which is better for this use, but an alternative would be to keep
the method id as a thread local value. Of course, there's some overhead to using
TLS too. For instance, every thread needs to redo the lookup.

[bulach, 2012-10-02 15:28:29]

thanks sami!
the results I had are in the patch description:
[ERROR:jni_android_unittest.cc(124)] JNI MT-21424
[ERROR:jni_android_unittest.cc(126)] JNI Non-MT-12878

MT => with the get function, Non-MT the straightforward call. numbers in
microseconds.. :)

more inline:

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

http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h#new...
base/android/jni_android.h:149: EXCEPTIONCHECK_YES,
On 2012/10/02 15:14:05, Sami wrote:
> Super-nit: NO=0, YES=1 would seem more logical.

:) done..

http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h#new...
base/android/jni_android.h:159: subtle::AtomicWord* atomic =
reinterpret_cast<subtle::AtomicWord*>(
On 2012/10/02 15:14:05, Sami wrote:
> I'm not sure which is better for this use, but an alternative would be to keep
> the method id as a thread local value. Of course, there's some overhead to
using
> TLS too. For instance, every thread needs to redo the lookup.

hmm... we have base::internal::ThreadLocalPointer, but I presume the lookup /
set is more expensive than the atomic load / store?
of course, I can try to measure that too if you want.. :)

[Sami, 2012-10-02 15:54:48]

On 2012/10/02 15:28:29, bulach wrote:
> thanks sami!
> the results I had are in the patch description:
> [ERROR:jni_android_unittest.cc(124)] JNI MT-21424
> [ERROR:jni_android_unittest.cc(126)] JNI Non-MT-12878
> 
> MT => with the get function, Non-MT the straightforward call. numbers in
> microseconds.. :)

Oh, sorry I missed those. I'm surprised the overhead is that clearly measurable.
Maybe it would be more reliable to call some Java method that doesn't allocate
memory to rule out the GC, e.g., Math.abs().

> more inline:
> 
> http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h
> File base/android/jni_android.h (right):
> 
>
http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h#new...
> base/android/jni_android.h:149: EXCEPTIONCHECK_YES,
> On 2012/10/02 15:14:05, Sami wrote:
> > Super-nit: NO=0, YES=1 would seem more logical.
> 
> :) done..
> 
>
http://codereview.chromium.org/11038015/diff/1/base/android/jni_android.h#new...
> base/android/jni_android.h:159: subtle::AtomicWord* atomic =
> reinterpret_cast<subtle::AtomicWord*>(
> On 2012/10/02 15:14:05, Sami wrote:
> > I'm not sure which is better for this use, but an alternative would be to
keep
> > the method id as a thread local value. Of course, there's some overhead to
> using
> > TLS too. For instance, every thread needs to redo the lookup.
> 
> hmm... we have base::internal::ThreadLocalPointer, but I presume the lookup /
> set is more expensive than the atomic load / store?
> of course, I can try to measure that too if you want.. :)

Nah, don't bother on my account :) I'd guess it is much more expensive too.

[bulach, 2012-10-02 16:41:25]

a-ha! indeed, using Math.abs and avoiding all the ScopedLocalRef (and possibly
GC) gets a much finer grained value:
[ERROR:jni_android_unittest.cc(125)] JNI MT-2197
[ERROR:jni_android_unittest.cc(127)] JNI Non-MT-1831

[bulach, 2012-10-02 17:19:39]

ok, I cleaned up the code and added all the tests, this is ready for real
review. :) ptal!

[joth, 2012-10-02 17:47:42]

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:14: #include "base/logging.h"
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.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:141: class LazyMethodID {
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)

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:151: };
enum duplication from the previous patch feels findof odd

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:154: static void Get(JNIEnv* env,
why is this in a class?

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:158: jmethodID* method_id) {
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.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:160: method_id);
nit: could compile assert that sizeof(AtomicWord) >= sizeof(jmethodID)

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:163: return;
need to document on the function that the caller is responsible to zero-init the
method id storage.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:164: jmethodID id = method_type == METHODTYPE_STATIC
?
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)

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:165: env->GetStaticMethodID(clazz, method_name,
jni_signature) :
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)

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:173: env->ExceptionClear();
can we call an existing GetMethodID blah class here?

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:175: base::subtle::Acquire_Store(
This needs to be a Release_Store

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
File base/android/jni_android_unittest.cc (right):

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:36: 
\n nit

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:37: static jmethodID g_method_id = 0;
this should be declared as an AtomicWord

nit: static not needed in anon namespace

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:45: &g_method_id);
are you specifically using a global method id rather than just declare it in the
test on stack and pass through as a param?

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:52: }
AIUI it's preferred to put single-use anon methods like these down immediately
before the test that uses them. (fine for a file to have multiple anon namespace
blocks)

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:111: TEST(JNIAndroidMicrobenchmark,
MethodId) {
in net/ they put micro benchmarks into a different test target (and don't run
every day). I think these are fast enough running to not be an issue in here
though? I'm pretty sure base has some already but I can't find now...

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:129: LOG(ERROR) << "JNI " << o;
can you include in comment here as to typical results (and what machine you ran
it on)

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

thanks joth! I have to leave now, but a few questions to avoid another around
the globe lag :)

I'll address all the comments tomorrow.

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:151: };
On 2012/10/02 17:47:43, joth wrote:
> enum duplication from the previous patch feels findof odd

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:158: jmethodID* method_id) {
On 2012/10/02 17:47:43, joth wrote:
> 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.

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:38]

Hmmm Note you *could* have LazyMethodID hold the AtmoicWord as a member, but
you'd need to make it a struct (all public data members) in order to allow
linker initialization of the instances. (Same as what I did for LazyInstance)
e.g.:
static LazyMethodID g_method_Foo_BarBaz = {0};

?

otherwise, see my comments below on your specific question...

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:158: jmethodID* method_id) {
On 2012/10/02 18:20:42, bulach wrote:
> On 2012/10/02 17:47:43, joth wrote:
> > 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.
> 
> 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?

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]

thanks joth!
latest changes include:

1. using an AtomicWord and returning the jmethodID as per your suggestion.

2. split the class into its own file, with its own test...
I kept the template API as it seems simpler.

3. if you agree it's simpler this way rather than having N different variations
of the methods laying around, then I'll cleanup the existing Get(Static)MethodID
as a follow up.

more inline:

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:14: #include "base/logging.h"
On 2012/10/02 17:47:43, joth wrote:
> 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.

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:141: class LazyMethodID {
On 2012/10/02 17:47:43, joth wrote:
> 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)

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:154: static void Get(JNIEnv* env,
On 2012/10/02 17:47:43, joth wrote:
> why is this in a class?

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:158: jmethodID* method_id) {
On 2012/10/02 18:30:39, joth wrote:
> On 2012/10/02 18:20:42, bulach wrote:
> > On 2012/10/02 17:47:43, joth wrote:
> > > 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.
> > 
> > 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?
> 
> 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) {
> ...
> }
> 

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:160: method_id);
On 2012/10/02 17:47:43, joth wrote:
> nit: could compile assert that sizeof(AtomicWord) >= sizeof(jmethodID)

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:163: return;
On 2012/10/02 17:47:43, joth wrote:
> need to document on the function that the caller is responsible to zero-init
the
> method id storage.

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:164: jmethodID id = method_type == METHODTYPE_STATIC
?
On 2012/10/02 17:47:43, joth wrote:
> 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)

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:165: env->GetStaticMethodID(clazz, method_name,
jni_signature) :
On 2012/10/02 17:47:43, joth wrote:
> 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)

so far, the caller seems to always have one.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:173: env->ExceptionClear();
On 2012/10/02 17:47:43, joth wrote:
> can we call an existing GetMethodID blah class here?

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

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android.h#...
base/android/jni_android.h:175: base::subtle::Acquire_Store(
On 2012/10/02 17:47:43, joth wrote:
> This needs to be a Release_Store

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
File base/android/jni_android_unittest.cc (right):

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:36: 
On 2012/10/02 17:47:43, joth wrote:
> \n nit

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:37: static jmethodID g_method_id = 0;
On 2012/10/02 17:47:43, joth wrote:
> this should be declared as an AtomicWord
> 
> nit: static not needed in anon namespace

Done.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:45: &g_method_id);
On 2012/10/02 17:47:43, joth wrote:
> are you specifically using a global method id rather than just declare it in
the
> test on stack and pass through as a param?

yes. I'm trying to reproduce as much as possible the same code path that the
(new) generated JNI code would call versus the old one.

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:52: }
On 2012/10/02 17:47:43, joth wrote:
> AIUI it's preferred to put single-use anon methods like these down immediately
> before the test that uses them. (fine for a file to have multiple anon
namespace
> blocks)

moved to its own file..

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:111: TEST(JNIAndroidMicrobenchmark,
MethodId) {
On 2012/10/02 17:47:43, joth wrote:
> in net/ they put micro benchmarks into a different test target (and don't run
> every day). I think these are fast enough running to not be an issue in here
> though? I'm pretty sure base has some already but I can't find now...

yeah, the extra target doesn't seem to be worth the hassle..

http://codereview.chromium.org/11038015/diff/7001/base/android/jni_android_un...
base/android/jni_android_unittest.cc:129: LOG(ERROR) << "JNI " << o;
On 2012/10/02 17:47:43, joth wrote:
> can you include in comment here as to typical results (and what machine you
ran
> it on)

good point! done

[joth, 2012-10-03 17:31:52]

> 2. split the class into its own file, with its own test...
> I kept the template API as it seems simpler.

See comment below... I don't have strong feeling on template vs non-template,
but either way I'd prefer the method bodies to be in .cc and I think this will
be cause for confusion to other readers in future that we now have 3 sets of
functions split across 2 headers, all for getting method IDs... :-/ (and field
IDs have their own variants of the same).

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_android_u...
File base/android/jni_android_unittest.cc (right):

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_android_u...
base/android/jni_android_unittest.cc:8: #include "base/logging.h"
remove?

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_generator...
File base/android/jni_generator/golden_sample_for_tests_jni.h (right):

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_generator...
base/android/jni_generator/golden_sample_for_tests_jni.h:21: using
base::android::ScopedJavaLocalRef;
(OT for this CL, but note this is a bug: shouldn't use 'using' in a header file.
Ditto for anon namespace below.)

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_generator...
base/android/jni_generator/golden_sample_for_tests_jni.h:114:
base::android::MethodID::LazyGet<
nit: can this go on the same line as "jmethodID id =" ?

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id.h
File base/android/jni_method_id.h (right):

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
base/android/jni_method_id.h:18: // This class is a wrapper for JNIEnv
Get(Static)MethodID.
call out this isn't a class at all, just a namespace. (Why isn't it a namespace.
But given we're in double depth namespace already, we hardly need a third
layer...)

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
base/android/jni_method_id.h:51: return id;
I'd still prefer these method bodies into a .cc file, and keep the header just
for function declarations (templated in class or otherwise -- that I don't care
so much on).

Splitting the stuff across multiple headers (jni_android.h vs jni_method_id.h)
is nice in theory, but doesn't give us much if in practice every user has to
include both anyway. Also as it stands it's a bit random that classes and fields
and one way of getting methods is in the former, but 2 other ways of getting
methods is in the latter. Cleaning that up is going to be a bundle of churn that
doesn't gain much now and just risks more breakages while we still have forks...
so on balance IMO it's easier to just keep the single header file at this point.

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
base/android/jni_method_id.h:67: subtle::AtomicWord value =
base::subtle::Acquire_Load(atomic_method_id);
FWIW, I realized this doesn't need to be an 'acquire'. It can be straight
no-memory barrier load.
(It slightly increases the risk of race causing double init, but at trade off of
each subsequent call being a bit faster)

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
base/android/jni_method_id.h:72: base::subtle::Acquire_Store(
nit: C++ style is to align params with open paren where possible

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
base/android/jni_method_id.h:76: };
if you leave as a class, DISALLOW_IMPLICIT_CTOR and hide the DTOR too.

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
File base/android/jni_method_id_unittest.cc (right):

http://codereview.chromium.org/11038015/diff/16001/base/android/jni_method_id...
base/android/jni_method_id_unittest.cc:42: o += LazyMethodIDCall(env,
clazz.obj(), i);
depending on your goals, this test is skewed against LazyMethodIDCall as that
has to do the initial JNIEnv::GetMethodID() call, where as non-lazy version
below gets this called "for free" on startup.

Put another-way -- this cost on Lazy version is amortized as the size of the
loop is increased (e.g. 1024 -> 1M), whereas the one below should be "constant"
regardless of the iterations.

http://codereview.chromium.org/11038015/diff/16001/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/11038015/diff/16001/base/base.gyp#newcode370
base/base.gyp:370: 'android/jni_method_id_unittest.cc',
nit: should add jni_method_id.h somewhere in here too.

[bulach, 2012-10-03 17:37:31]

sorry, I should clarify: I *will* delete all method id related functions from
jni_android and keep only the new file.. does it make sense?

[joth, 2012-10-03 17:51:00]

On 3 October 2012 10:37, <bulach@chromium.org> wrote:

> sorry, I should clarify: I *will* delete all method id related functions
> from
> jni_android and keep only the new file.. does it make sense?
>
> Depends how much churn will that cause in the rest of the code? I guess if
method names remain same then it's just include file churn. Still think we
can live fine without that churn now.
Also it seems non-obvious  to have to look for GetMethod and
GetMethodIDFromClassName in one header file, but leave both GetClass and
GetField in another?

[bulach, 2012-10-03 17:54:11]

we could potentially split those too, but yes, probably way too much churn
for now...

tomorrow I'll get back to your original idea of having N public functions,
and just have this class as an implementation detail then. All methods will
live in the existing jni_android.* file.



On Wed, Oct 3, 2012 at 6:50 PM, Jonathan Dixon <joth@chromium.org> wrote:

>
>
>
> On 3 October 2012 10:37, <bulach@chromium.org> wrote:
>
>> sorry, I should clarify: I *will* delete all method id related functions
>> from
>> jni_android and keep only the new file.. does it make sense?
>>
>> Depends how much churn will that cause in the rest of the code? I guess
> if method names remain same then it's just include file churn. Still think
> we can live fine without that churn now.
> Also it seems non-obvious  to have to look for GetMethod and
> GetMethodIDFromClassName in one header file, but leave both GetClass and
> GetField in another?
>
>
>

[joth, 2012-10-03 17:59:05]

To be clear: if you feel strongly on leaving templating in the API, I'm
fine with that - do what's easiest for the caller to use. But if you do,
just put the template class implementation into a .cc. (remember - you
don't have to define template methods in a .h when you know exactly which
concrete template instances will be needed, as we do here: just use
explicit template instantiation: http://www.cplusplus.com/articles/1C75fSEw/
)


On 3 October 2012 10:54, Marcus Bulach <bulach@chromium.org> wrote:

> we could potentially split those too, but yes, probably way too much churn
> for now...
>
> tomorrow I'll get back to your original idea of having N public functions,
> and just have this class as an implementation detail then. All methods will
> live in the existing jni_android.* file.
>
>
>
> On Wed, Oct 3, 2012 at 6:50 PM, Jonathan Dixon <joth@chromium.org> wrote:
>
>>
>>
>>
>> On 3 October 2012 10:37, <bulach@chromium.org> wrote:
>>
>>> sorry, I should clarify: I *will* delete all method id related functions
>>> from
>>> jni_android and keep only the new file.. does it make sense?
>>>
>>> Depends how much churn will that cause in the rest of the code? I guess
>> if method names remain same then it's just include file churn. Still think
>> we can live fine without that churn now.
>>  Also it seems non-obvious  to have to look for GetMethod and
>> GetMethodIDFromClassName in one header file, but leave both GetClass and
>> GetField in another?
>>
>>
>>
>

[bulach, 2012-10-04 16:19:00]

joth: apologies for the biggie.. :(

Summary:
1. Followed your suggestion to move the impl to the .cc, and have explicit
template instation there.

2. This allowed a reasonably smaller API surface, i.e., just the templated Get /
LazyGet methods.

3. I removed all other variations of Get(Static)MethodID, including ones that
would take a jclass vs ScopedJavaRef<jclass> (useless little helper).. I also
removed the HasMethod.

please, let me know your thoughts on this.

[joth, 2012-10-04 17:59:59]

The Lazy getter all LGTM - thanks much for this improvement!
I have some questions on switching rest of code to template API....  if you want
to cut the ongoing review round-trips then maybe split that out to another CL.
(if we do go with that, makes sense for you to cherry-pick this downstream as
there maybe more interesting uses of MethodID there)

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

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.cc...
base/android/jni_android.cc:218: const char* jni_signature);
fwiw the ::Get() instantiations are not strictly needed as LazyGet calls Get()
so they'll happen as side effect anyway. (Probably best to keep them all though
for clarity?)

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

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#...
base/android/jni_android.h:85: // ::Get() above. If there's a race, it's ok
since the values are the same
nit: if there's a race... is all implementation detail, moving into the .cc as
not needed on the API. (could just call out here that it's fine to
simultaneously call this on multiple threads referencing the same
|atomic_method_id|

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#...
base/android/jni_android.h:98: // MethodID() above avoids a class lookup, but
does not cache results.
nit: MethodID above is not a function. MethodID::Get()

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
File base/android/jni_generator/golden_sample_for_tests_jni.h (right):

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
base/android/jni_generator/golden_sample_for_tests_jni.h:86: 
nit: still have 2 spurious \n here

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (left):

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:610: GetMethodIDsImpl(env);
one thing I did remember this morning: currently if there's some build/config
problem we get a crash on startup if any (app custom class) method can't be
found. with this change, we'll only get the crash way down line when method is
called, so will rely on 100% code coverage to find such issues...
would is be worth leaving in a debug-only variant of GetMethodIDs here that
merely DCHECKS(HasMethod(...)) for each of the (non-system) method IDs? i.e.
leave the registrations themselves to use LazyGet, but still have the benefit of
debug startup sanity check on this?

http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/andr...
File chrome/browser/history/android/sqlite_cursor.cc (right):

http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/andr...
chrome/browser/history/android/sqlite_cursor.cc:65: MethodID::METHODTYPE_NORMAL,
MethodID::EXCEPTIONCHECK_NO>(
For the non-lazy API I'm ambivalent about switching to template API. I see the
consistency argument, but the EXCEPTIONCHECK_YES is 90% irrelevant noise, and in
the odd place where it's important like here I completely missed the fact this
method may be chucking a NULL pointer back at us. (And I even was going to ask
whether the if(!method_id) below is spurious)

TL;DR: GetFooOrNull() is much easier for my eyes to spot, than the difference
between _YES vs _NO in a param I trained myself to ignore.

http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_unittest.cc:36: #endif
nit: put this down inside the test where you use it. 'using' statements are fine
(in fact, nice) scoped inside a fn.

http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_...
File webkit/glue/fling_animator_impl_android.cc (right):

http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_...
webkit/glue/fling_animator_impl_android.cc:11: using namespace base::android;
oh! this is not allowed is it?

http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_...
webkit/glue/fling_animator_impl_android.cc:21: jmethodID constructor =
MethodID::Get<
should at least add the explicit  'using ..MethodID' anyway...

[bulach, 2012-10-04 18:58:17]

many thanks joth! comments addressed and some other clarified..

I'm not in a particular rush, so I guess it'll be much simpler to keep this as a
monolithic patch so that I can move downstream and fix it up in one go after it
lands..

let me know your thoughts on the few remaining questions..

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

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.cc...
base/android/jni_android.cc:218: const char* jni_signature);
On 2012/10/04 17:59:59, joth wrote:
> fwiw the ::Get() instantiations are not strictly needed as LazyGet calls Get()
> so they'll happen as side effect anyway. (Probably best to keep them all
though
> for clarity?)

I'd love to delete, but I think this makes it clearer and will later avoid heads
being scratched on why one and not the other? :)

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

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#...
base/android/jni_android.h:85: // ::Get() above. If there's a race, it's ok
since the values are the same
On 2012/10/04 17:59:59, joth wrote:
> nit: if there's a race... is all implementation detail, moving into the .cc as
> not needed on the API. (could just call out here that it's fine to
> simultaneously call this on multiple threads referencing the same
> |atomic_method_id|

Done.

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#...
base/android/jni_android.h:98: // MethodID() above avoids a class lookup, but
does not cache results.
On 2012/10/04 17:59:59, joth wrote:
> nit: MethodID above is not a function. MethodID::Get()

Done.

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
File base/android/jni_generator/golden_sample_for_tests_jni.h (right):

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
base/android/jni_generator/golden_sample_for_tests_jni.h:86: 
On 2012/10/04 17:59:59, joth wrote:
> nit: still have 2 spurious \n here

Done.

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (left):

http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:610: GetMethodIDsImpl(env);
On 2012/10/04 17:59:59, joth wrote:
> one thing I did remember this morning: currently if there's some build/config
> problem we get a crash on startup if any (app custom class) method can't be
> found. with this change, we'll only get the crash way down line when method is
> called, so will rely on 100% code coverage to find such issues...
> would is be worth leaving in a debug-only variant of GetMethodIDs here that
> merely DCHECKS(HasMethod(...)) for each of the (non-system) method IDs? i.e.
> leave the registrations themselves to use LazyGet, but still have the benefit
of
> debug startup sanity check on this?

I agree in principle, but for app classes, how can there be a mismatch?
 
I mean, unless we have a broken build system :) but other than that, the JNI
should surely match the classes that are in the APK?

http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/andr...
File chrome/browser/history/android/sqlite_cursor.cc (right):

http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/andr...
chrome/browser/history/android/sqlite_cursor.cc:65: MethodID::METHODTYPE_NORMAL,
MethodID::EXCEPTIONCHECK_NO>(
On 2012/10/04 17:59:59, joth wrote:
> For the non-lazy API I'm ambivalent about switching to template API. I see the
> consistency argument, but the EXCEPTIONCHECK_YES is 90% irrelevant noise, and
in
> the odd place where it's important like here I completely missed the fact this
> method may be chucking a NULL pointer back at us. (And I even was going to ask
> whether the if(!method_id) below is spurious)
> 
> TL;DR: GetFooOrNull() is much easier for my eyes to spot, than the difference
> between _YES vs _NO in a param I trained myself to ignore.

well, a few steps back :)
aren't your eyes ignoring just because this patch is massive?

I mean, going forward, both reviewers and writers should:
- not write custom JNI in the first place! :)
(I do intend to get rid of these at some point..)

- if they do, they should be really careful.

I'd rather keep a single consistent API, otherwise we'll end up with that
multitude of methods with ambiguous documentation quite quickly... wdyt?

http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_unittest.cc:36: #endif
On 2012/10/04 17:59:59, joth wrote:
> nit: put this down inside the test where you use it. 'using' statements are
fine
> (in fact, nice) scoped inside a fn.

Done (indeed, there was already an "using" clause in the fn, thanks!)

http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_...
File webkit/glue/fling_animator_impl_android.cc (right):

http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_...
webkit/glue/fling_animator_impl_android.cc:11: using namespace base::android;
On 2012/10/04 17:59:59, joth wrote:
> oh! this is not allowed is it?

aarrggghh!!! :)
done.

http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_...
webkit/glue/fling_animator_impl_android.cc:21: jmethodID constructor =
MethodID::Get<
On 2012/10/04 17:59:59, joth wrote:
> should at least add the explicit  'using ..MethodID' anyway...

added the correct "using" above..

[nyquist, 2012-10-04 19:05:48]

http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/andr...
File chrome/browser/history/android/sqlite_cursor.cc (right):

http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/andr...
chrome/browser/history/android/sqlite_cursor.cc:65: MethodID::METHODTYPE_NORMAL,
MethodID::EXCEPTIONCHECK_NO>(
On 2012/10/04 18:58:17, bulach wrote:
> On 2012/10/04 17:59:59, joth wrote:
> > For the non-lazy API I'm ambivalent about switching to template API. I see
the
> > consistency argument, but the EXCEPTIONCHECK_YES is 90% irrelevant noise,
and
> in
> > the odd place where it's important like here I completely missed the fact
this
> > method may be chucking a NULL pointer back at us. (And I even was going to
ask
> > whether the if(!method_id) below is spurious)
> > 
> > TL;DR: GetFooOrNull() is much easier for my eyes to spot, than the
difference
> > between _YES vs _NO in a param I trained myself to ignore.
> 
> well, a few steps back :)
> aren't your eyes ignoring just because this patch is massive?
> 
> I mean, going forward, both reviewers and writers should:
> - not write custom JNI in the first place! :)
> (I do intend to get rid of these at some point..)
> 
> - if they do, they should be really careful.
> 
> I'd rather keep a single consistent API, otherwise we'll end up with that
> multitude of methods with ambiguous documentation quite quickly... wdyt?

So, I kind of agree with joth that it might be hard to remember that by passing
the EXCEPTIONCHECK_NO template argument, the method can return null. Maybe the
argument should be clearer?
For example, change it to something like:
ALLOW_NULL_RESULT / DISALLOW_NULL_RESULT ?
Or use any type of good English language tricks to clarify the meaning of the
enum value, and what implications it has?

[joth, 2012-10-04 19:21:52]

On 4 October 2012 11:58, <bulach@chromium.org> wrote:

> many thanks joth! comments addressed and some other clarified..
>
> I'm not in a particular rush, so I guess it'll be much simpler to keep
> this as a
> monolithic patch so that I can move downstream and fix it up in one go
> after it
> lands..
>
> let me know your thoughts on the few remaining questions..
>
>
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_android.cc<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.cc>
> File base/android/jni_android.cc (right):
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_android.cc#**newcode218<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.cc#newcode218>
> base/android/jni_android.cc:**218: const char* jni_signature);
> On 2012/10/04 17:59:59, joth wrote:
>
>> fwiw the ::Get() instantiations are not strictly needed as LazyGet
>>
> calls Get()
>
>> so they'll happen as side effect anyway. (Probably best to keep them
>>
> all though
>
>> for clarity?)
>>
>
> I'd love to delete, but I think this makes it clearer and will later
> avoid heads being scratched on why one and not the other? :)
>
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_android.h<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h>
> File base/android/jni_android.h (right):
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_android.h#**newcode85<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#newcode85>
> base/android/jni_android.h:85: // ::Get() above. If there's a race, it's
> ok since the values are the same
> On 2012/10/04 17:59:59, joth wrote:
>
>> nit: if there's a race... is all implementation detail, moving into
>>
> the .cc as
>
>> not needed on the API. (could just call out here that it's fine to
>> simultaneously call this on multiple threads referencing the same
>> |atomic_method_id|
>>
>
> Done.
>
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_android.h#**newcode98<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#newcode98>
> base/android/jni_android.h:98: // MethodID() above avoids a class
> lookup, but does not cache results.
> On 2012/10/04 17:59:59, joth wrote:
>
>> nit: MethodID above is not a function. MethodID::Get()
>>
>
> Done.
>
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_generator/golden_**sample_for_tests_jni.h<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/golden_sample_for_tests_jni.h>
> File base/android/jni_generator/**golden_sample_for_tests_jni.h (right):
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_generator/golden_**sample_for_tests_jni.h#**newcode86<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/golden_sample_for_tests_jni.h#newcode86>
> base/android/jni_generator/**golden_sample_for_tests_jni.h:**86:
> On 2012/10/04 17:59:59, joth wrote:
>
>> nit: still have 2 spurious \n here
>>
>
> Done.
>
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_generator/jni_**generator.py<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/jni_generator.py>
> File base/android/jni_generator/**jni_generator.py (left):
>
> http://codereview.chromium.**org/11038015/diff/9004/base/**
>
android/jni_generator/jni_**generator.py#oldcode610<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/jni_generator.py#oldcode610>
> base/android/jni_generator/**jni_generator.py:610: GetMethodIDsImpl(env);
> On 2012/10/04 17:59:59, joth wrote:
>
>> one thing I did remember this morning: currently if there's some
>>
> build/config
>
>> problem we get a crash on startup if any (app custom class) method
>>
> can't be
>
>> found. with this change, we'll only get the crash way down line when
>>
> method is
>
>> called, so will rely on 100% code coverage to find such issues...
>> would is be worth leaving in a debug-only variant of GetMethodIDs here
>>
> that
>
>> merely DCHECKS(HasMethod(...)) for each of the (non-system) method
>>
> IDs? i.e.
>
>> leave the registrations themselves to use LazyGet, but still have the
>>
> benefit of
>
>> debug startup sanity check on this?
>>
>
> I agree in principle, but for app classes, how can there be a mismatch?
>
> I mean, unless we have a broken build system :) but other than that, the
> JNI should surely match the classes that are in the APK?
>
> exactly - this would only happen on some config / APK mismatch type error.
I have hit it myself a few times though (failing to get the right thing
rebuilt in right order) and I valued the benefit of being told I'd screwed
up immediately on startup rather than randomly when executing some deep
corner of functionality.
So just a trade off of constant cost on debug startup and jni_generator
maintenance, vs very occasional developer error time-saver.  I'm OK with
whichever you choose.




>
> http://codereview.chromium.**org/11038015/diff/9004/chrome/**
>
browser/history/android/**sqlite_cursor.cc<http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/android/sqlite_cursor.cc>
> File chrome/browser/history/**android/sqlite_cursor.cc (right):
>
> http://codereview.chromium.**org/11038015/diff/9004/chrome/**
>
browser/history/android/**sqlite_cursor.cc#newcode65<http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/android/sqlite_cursor.cc#newcode65>
> chrome/browser/history/**android/sqlite_cursor.cc:65:
> MethodID::METHODTYPE_NORMAL, MethodID::EXCEPTIONCHECK_NO>(
> On 2012/10/04 17:59:59, joth wrote:
>
>> For the non-lazy API I'm ambivalent about switching to template API. I
>>
> see the
>
>> consistency argument, but the EXCEPTIONCHECK_YES is 90% irrelevant
>>
> noise, and in
>
>> the odd place where it's important like here I completely missed the
>>
> fact this
>
>> method may be chucking a NULL pointer back at us. (And I even was
>>
> going to ask
>
>> whether the if(!method_id) below is spurious)
>>
>
>  TL;DR: GetFooOrNull() is much easier for my eyes to spot, than the
>>
> difference
>
>> between _YES vs _NO in a param I trained myself to ignore.
>>
>
> well, a few steps back :)
> aren't your eyes ignoring just because this patch is massive?
>
> I mean, going forward, both reviewers and writers should:
> - not write custom JNI in the first place! :)
> (I do intend to get rid of these at some point..)
>
> True.


> - if they do, they should be really careful.
>
> sure, everyone should read carefully. But good APIs should also draw yours
attention to the most important points to read.

imagine if scoped_ptr didn't have a .release() method, but instead you had
to always call my_ptr.get<OWNERSHIPRETAINED_YES>()  or
 my_ptr. get<OWNERSHIPRETAINED_NO>()  ... my eyes wouldn't help but miss
the occasional _NO hidden among  a thousand _YESes





> I'd rather keep a single consistent API, otherwise we'll end up with
> that multitude of methods with ambiguous documentation quite quickly...
> wdyt?
>
>
guess it comes down to I'd be more comfortable with ordering:
- add new API along side old one
- fix up all users (up & downstream) to use jni_generator
- remove old API (no one now using it anyway)





>
> http://codereview.chromium.**org/11038015/diff/9004/ui/**
>
base/clipboard/clipboard_**unittest.cc<http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard_unittest.cc>
> File ui/base/clipboard/clipboard_**unittest.cc (right):
>
> http://codereview.chromium.**org/11038015/diff/9004/ui/**
>
base/clipboard/clipboard_**unittest.cc#newcode36<http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard_unittest.cc#newcode36>
> ui/base/clipboard/clipboard_**unittest.cc:36: #endif
> On 2012/10/04 17:59:59, joth wrote:
>
>> nit: put this down inside the test where you use it. 'using'
>>
> statements are fine
>
>> (in fact, nice) scoped inside a fn.
>>
>
> Done (indeed, there was already an "using" clause in the fn, thanks!)
>
>
> http://codereview.chromium.**org/11038015/diff/9004/webkit/**
>
glue/fling_animator_impl_**android.cc<http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_impl_android.cc>
> File webkit/glue/fling_animator_**impl_android.cc (right):
>
> http://codereview.chromium.**org/11038015/diff/9004/webkit/**
>
glue/fling_animator_impl_**android.cc#newcode11<http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_impl_android.cc#newcode11>
> webkit/glue/fling_animator_**impl_android.cc:11: using namespace
> base::android;
> On 2012/10/04 17:59:59, joth wrote:
>
>> oh! this is not allowed is it?
>>
>
> aarrggghh!!! :)
> done.
>
>
> http://codereview.chromium.**org/11038015/diff/9004/webkit/**
>
glue/fling_animator_impl_**android.cc#newcode21<http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_impl_android.cc#newcode21>
> webkit/glue/fling_animator_**impl_android.cc:21: jmethodID constructor =
> MethodID::Get<
> On 2012/10/04 17:59:59, joth wrote:
>
>> should at least add the explicit  'using ..MethodID' anyway...
>>
>
> added the correct "using" above..
>
>
http://codereview.chromium.**org/11038015/<http://codereview.chromium.org/110...
>



So, I kind of agree with joth that it might be hard to remember that by
> passing the EXCEPTIONCHECK_NO template argument, the method can return
> null. Maybe the argument should be clearer?
> For example, change it to something like:
> ALLOW_NULL_RESULT / DISALLOW_NULL_RESULT ?
> Or use any type of good English language tricks to clarify the meaning
> of the enum value, and what implications it has?


Yes, this could certainly help. Coupled with evidence that it's extremely
rare anyone manually writes code to use the methods (i.e. all code migrated
to jni_generator) I'd buy that :-)

[joth, 2012-10-05 05:46:47]

On 4 October 2012 12:21, Jonathan Dixon <joth@chromium.org> wrote:

>
>
>
> On 4 October 2012 11:58, <bulach@chromium.org> wrote:
>
>> many thanks joth! comments addressed and some other clarified..
>>
>> I'm not in a particular rush, so I guess it'll be much simpler to keep
>> this as a
>> monolithic patch so that I can move downstream and fix it up in one go
>> after it
>> lands..
>>
>> let me know your thoughts on the few remaining questions..
>>
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_android.cc<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.cc>
>> File base/android/jni_android.cc (right):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_android.cc#**newcode218<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.cc#newcode218>
>> base/android/jni_android.cc:**218: const char* jni_signature);
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> fwiw the ::Get() instantiations are not strictly needed as LazyGet
>>>
>> calls Get()
>>
>>> so they'll happen as side effect anyway. (Probably best to keep them
>>>
>> all though
>>
>>> for clarity?)
>>>
>>
>> I'd love to delete, but I think this makes it clearer and will later
>> avoid heads being scratched on why one and not the other? :)
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_android.h<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h>
>> File base/android/jni_android.h (right):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_android.h#**newcode85<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#newcode85>
>> base/android/jni_android.h:85: // ::Get() above. If there's a race, it's
>> ok since the values are the same
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> nit: if there's a race... is all implementation detail, moving into
>>>
>> the .cc as
>>
>>> not needed on the API. (could just call out here that it's fine to
>>> simultaneously call this on multiple threads referencing the same
>>> |atomic_method_id|
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_android.h#**newcode98<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_android.h#newcode98>
>> base/android/jni_android.h:98: // MethodID() above avoids a class
>> lookup, but does not cache results.
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> nit: MethodID above is not a function. MethodID::Get()
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_generator/golden_**sample_for_tests_jni.h<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/golden_sample_for_tests_jni.h>
>> File base/android/jni_generator/**golden_sample_for_tests_jni.h (right):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_generator/golden_**sample_for_tests_jni.h#**newcode86<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/golden_sample_for_tests_jni.h#newcode86>
>> base/android/jni_generator/**golden_sample_for_tests_jni.h:**86:
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> nit: still have 2 spurious \n here
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_generator/jni_**generator.py<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/jni_generator.py>
>> File base/android/jni_generator/**jni_generator.py (left):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/base/**
>>
android/jni_generator/jni_**generator.py#oldcode610<http://codereview.chromium.org/11038015/diff/9004/base/android/jni_generator/jni_generator.py#oldcode610>
>> base/android/jni_generator/**jni_generator.py:610: GetMethodIDsImpl(env);
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> one thing I did remember this morning: currently if there's some
>>>
>> build/config
>>
>>> problem we get a crash on startup if any (app custom class) method
>>>
>> can't be
>>
>>> found. with this change, we'll only get the crash way down line when
>>>
>> method is
>>
>>> called, so will rely on 100% code coverage to find such issues...
>>> would is be worth leaving in a debug-only variant of GetMethodIDs here
>>>
>> that
>>
>>> merely DCHECKS(HasMethod(...)) for each of the (non-system) method
>>>
>> IDs? i.e.
>>
>>> leave the registrations themselves to use LazyGet, but still have the
>>>
>> benefit of
>>
>>> debug startup sanity check on this?
>>>
>>
>> I agree in principle, but for app classes, how can there be a mismatch?
>>
>> I mean, unless we have a broken build system :) but other than that, the
>> JNI should surely match the classes that are in the APK?
>>
>> exactly - this would only happen on some config / APK mismatch type
> error. I have hit it myself a few times though (failing to get the right
> thing rebuilt in right order) and I valued the benefit of being told I'd
> screwed up immediately on startup rather than randomly when executing some
> deep corner of functionality.
> So just a trade off of constant cost on debug startup and jni_generator
> maintenance, vs very occasional developer error time-saver.  I'm OK with
> whichever you choose.
>
>
>
>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/chrome/**
>>
browser/history/android/**sqlite_cursor.cc<http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/android/sqlite_cursor.cc>
>> File chrome/browser/history/**android/sqlite_cursor.cc (right):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/chrome/**
>>
browser/history/android/**sqlite_cursor.cc#newcode65<http://codereview.chromium.org/11038015/diff/9004/chrome/browser/history/android/sqlite_cursor.cc#newcode65>
>> chrome/browser/history/**android/sqlite_cursor.cc:65:
>> MethodID::METHODTYPE_NORMAL, MethodID::EXCEPTIONCHECK_NO>(
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> For the non-lazy API I'm ambivalent about switching to template API. I
>>>
>> see the
>>
>>> consistency argument, but the EXCEPTIONCHECK_YES is 90% irrelevant
>>>
>> noise, and in
>>
>>> the odd place where it's important like here I completely missed the
>>>
>> fact this
>>
>>> method may be chucking a NULL pointer back at us. (And I even was
>>>
>> going to ask
>>
>>> whether the if(!method_id) below is spurious)
>>>
>>
>>  TL;DR: GetFooOrNull() is much easier for my eyes to spot, than the
>>>
>> difference
>>
>>> between _YES vs _NO in a param I trained myself to ignore.
>>>
>>
>> well, a few steps back :)
>> aren't your eyes ignoring just because this patch is massive?
>>
>> I mean, going forward, both reviewers and writers should:
>> - not write custom JNI in the first place! :)
>> (I do intend to get rid of these at some point..)
>>
>> True.
>
>
>> - if they do, they should be really careful.
>>
>> sure, everyone should read carefully. But good APIs should also draw
> yours attention to the most important points to read.
>
> imagine if scoped_ptr didn't have a .release() method, but instead you had
> to always call my_ptr.get<OWNERSHIPRETAINED_YES>()  or
>  my_ptr. get<OWNERSHIPRETAINED_NO>()  ... my eyes wouldn't help but miss
> the occasional _NO hidden among  a thousand _YESes
>
>
>
>
>
>> I'd rather keep a single consistent API, otherwise we'll end up with
>> that multitude of methods with ambiguous documentation quite quickly...
>> wdyt?
>>
>>
> guess it comes down to I'd be more comfortable with ordering:
> - add new API along side old one
> - fix up all users (up & downstream) to use jni_generator
> - remove old API (no one now using it anyway)
>
>
>
>
>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/ui/**
>>
base/clipboard/clipboard_**unittest.cc<http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard_unittest.cc>
>> File ui/base/clipboard/clipboard_**unittest.cc (right):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/ui/**
>>
base/clipboard/clipboard_**unittest.cc#newcode36<http://codereview.chromium.org/11038015/diff/9004/ui/base/clipboard/clipboard_unittest.cc#newcode36>
>> ui/base/clipboard/clipboard_**unittest.cc:36: #endif
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> nit: put this down inside the test where you use it. 'using'
>>>
>> statements are fine
>>
>>> (in fact, nice) scoped inside a fn.
>>>
>>
>> Done (indeed, there was already an "using" clause in the fn, thanks!)
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/webkit/**
>>
glue/fling_animator_impl_**android.cc<http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_impl_android.cc>
>> File webkit/glue/fling_animator_**impl_android.cc (right):
>>
>> http://codereview.chromium.**org/11038015/diff/9004/webkit/**
>>
glue/fling_animator_impl_**android.cc#newcode11<http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_impl_android.cc#newcode11>
>> webkit/glue/fling_animator_**impl_android.cc:11: using namespace
>> base::android;
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> oh! this is not allowed is it?
>>>
>>
>> aarrggghh!!! :)
>> done.
>>
>>
>> http://codereview.chromium.**org/11038015/diff/9004/webkit/**
>>
glue/fling_animator_impl_**android.cc#newcode21<http://codereview.chromium.org/11038015/diff/9004/webkit/glue/fling_animator_impl_android.cc#newcode21>
>> webkit/glue/fling_animator_**impl_android.cc:21: jmethodID constructor =
>> MethodID::Get<
>> On 2012/10/04 17:59:59, joth wrote:
>>
>>> should at least add the explicit  'using ..MethodID' anyway...
>>>
>>
>> added the correct "using" above..
>>
>>
http://codereview.chromium.**org/11038015/<http://codereview.chromium.org/110...
>>
>
>
>
> So, I kind of agree with joth that it might be hard to remember that by
>> passing the EXCEPTIONCHECK_NO template argument, the method can return
>> null. Maybe the argument should be clearer?
>> For example, change it to something like:
>> ALLOW_NULL_RESULT / DISALLOW_NULL_RESULT ?
>> Or use any type of good English language tricks to clarify the meaning
>> of the enum value, and what implications it has?
>
>
> Yes, this could certainly help. Coupled with evidence that it's extremely
> rare anyone manually writes code to use the methods (i.e. all code migrated
> to jni_generator) I'd buy that :-)
>
>
>

Actually..... just thought: can't we just get rid of this template param
altogether now?  isn't the original point of adding  the Lazy Method Getter
that we then don't need to have a version that returns NULL? We'd survived
without it until now, so if we can drop it again that would be my favorite
solution of all. (The one usage in SQL cursor, we already agreed in
previous iteration was fine to change to a non-conditional lookup anyway).

Also, EXCEPTIONCHECK_NO is an odd thing to use with the lazy version in any
case: there we have already defined NULL value (in the AtomicWord) to
indicate we've not yet done the lazy registration, so we'd end up doing the
lazy registration repeatedly over and again. In the rare case someone
really wants to deal with a potentially non-existant method, you really
don't need the choice of lazy and non-lazy getters for the same. (Actually,
the one primary use we have for dynamic reflection lookup of methods is in
the JavaBridge, and that uses the GetMethodIDForClass variants anyway; that
already deals with thread-safe lookups and recording NULL values if the
method does not exist so will only look it up once in all cases).

[bulach, 2012-10-05 10:41:40]

double duh! :) thanks both: indeed, we can always check exceptions, there's no
need for that param.
no one should call unavailable methods (yet)..

I removed the HasMethod from sqllite_cursor...

as for how to proceed: this patch reduces from 7 public methods to just two
(well, parametrized, but still). the documentation seems clearer, and the
changes in other files are mechanical (plus necessary cleanups, such as
unecessary / wrong using clauses)..

let me know if you prefer me to split, but I still think a monolithic cleanup
will make merging simpler..

[joth, 2012-10-05 15:24:57]

lgtm - no problem with the wholesale change now API details are sorted (was just
trying to decouple the separate discussions).

just one minor latent thought...
Thanks!

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

http://codereview.chromium.org/11038015/diff/26026/base/android/jni_android.h...
base/android/jni_android.h:66: METHODTYPE_NORMAL,
- METHOD seems redundant now this is in class MethodID (and that will probably
appear twice already where this is used)
- NORMAL => INSTANCE will help give more context at the call site. So:-

TYPE_STATIC,
TYPE_INSTANCE,

wdyt?

[bulach, 2012-10-05 15:58:15]

used your idea, thanks joth!
tommy: would you like to take a final look into it as well?
once we're all happy I'll add the respective OWNERS in the remaining files...

I hope I'll be able to land this early next week, and I already have a patch
ready downstream for the fix ups.

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

http://codereview.chromium.org/11038015/diff/26026/base/android/jni_android.h...
base/android/jni_android.h:66: METHODTYPE_NORMAL,
On 2012/10/05 15:24:57, joth wrote:
> - METHOD seems redundant now this is in class MethodID (and that will probably
> appear twice already where this is used)
> - NORMAL => INSTANCE will help give more context at the call site. So:-
> 
> TYPE_STATIC,
> TYPE_INSTANCE,
> 
> wdyt?

good idea joth, thanks!
done.

[joth, 2012-10-05 16:03:18]

lgtm

[bulach, 2012-10-08 08:49:19]

hey guys, sorry about the wide distribution.. We're changing a base/ API for
android, and there are some widespread (truly minor) changes.. the API change
itself has already been reviewed.

this requires OWNERS approval for:
tony: ui/base/clipboard and webkit
akalin: sync/
sky: chrome/browser/history
joi: chrome/browser/component
jar: net/

btw, if you're happy with a tbr for such changes, please let me know!

thanks!

[Jói, 2012-10-08 10:06:39]

LGTM for c/b/component

[sky, 2012-10-08 15:28:53]

c/b/h LGTM

[jar (doing other things), 2012-10-08 16:09:02]

for /net  patch set 8 LGTM

[tony, 2012-10-08 17:39:14]

rs LGTM

[joth, 2012-10-08 22:21:55]

+tim for sync/

[bulach, 2012-10-11 18:06:07]

+atwilson for sync/ OWNERS approval..

[commit-bot: I haz the power, 2012-10-16 11:22:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11038015/47001

[commit-bot: I haz the power, 2012-10-16 18:30:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11038015/47001

[commit-bot: I haz the power, 2012-10-16 18:35:15]

Change committed as 162186