[Android] Cache Java-based histogram lookup through RecordHistogram

[Android] Cache Java-based histogram lookup through RecordHistogram

Avoid JNI string translation for each histogrom update routed through
RecordHistogram by caching the histogram lookup. Use the Java string
handle as the key.

BUG=442300
Committed: https://crrev.com/825e324f2c26273ba8e60dba64e8b8739d0e3d85
Cr-Commit-Position: refs/heads/master@{#313174}

[jdduke (slow), 2015-01-23 19:18:15]

jdduke@chromium.org changed reviewers:
+ asvitkine@chromium.org, ppi@chromium.org

[jdduke (slow), 2015-01-23 19:18:15]

PTAL, thanks.

[jdduke (slow), 2015-01-23 19:26:55]

jdduke@chromium.org changed reviewers:
+ nyquist@chromium.org

[jdduke (slow), 2015-01-23 19:26:55]

On 2015/01/23 19:18:15, jdduke wrote:
> PTAL, thanks.

I guess my question is how confident can we be that the Java string pointer will
never change for the same key? I know the string pool *should* be used in such
cases, just want to make sure (not a Java expert =/). +nyquist for owner review.

[Alexei Svitkine (slow), 2015-01-23 19:31:51]

asvitkine@chromium.org changed reviewers:
- nyquist@chromium.org

[Alexei Svitkine (slow), 2015-01-23 19:31:52]

The C++ code looks good, but now that I think about this, I don't think this is
safe to do.

Even if jstring is the pointer to the string object (which also may not be
guaranteed... we have to check the docs), the JVM could compact and change the
underlying memory address, even if the string is interned. So this map isn't
safe, since it could store stale addresses.

We could either make the map live on the Java side (since the key would be the
String, it would continue to be consistent after compaction, etc) - but would be
slower since an actual string compare will need to happen when using map
operations.

Or, we could keep this on the C++ side, but use a stable identifier for the
string, for example  System.identityHashCode(str) - which we could pass as an
extra parameter to the JNI C++ code from the Java side.

[jdduke (slow), 2015-01-23 19:39:00]

On 2015/01/23 19:31:52, Alexei Svitkine wrote:
> The C++ code looks good, but now that I think about this, I don't think this
is
> safe to do.
> 
> Even if jstring is the pointer to the string object (which also may not be
> guaranteed... we have to check the docs), the JVM could compact and change the
> underlying memory address, even if the string is interned. So this map isn't
> safe, since it could store stale addresses.
> 
> We could either make the map live on the Java side (since the key would be the
> String, it would continue to be consistent after compaction, etc) - but would
be
> slower since an actual string compare will need to happen when using map
> operations.
> 
> Or, we could keep this on the C++ side, but use a stable identifier for the
> string, for example  System.identityHashCode(str) - which we could pass as an
> extra parameter to the JNI C++ code from the Java side.

Yeah, I suspected something like that. System.identityHashCode seems pretty
reasonable.

[jdduke (slow), 2015-01-23 20:16:37]

On 2015/01/23 19:39:00, jdduke wrote:
> On 2015/01/23 19:31:52, Alexei Svitkine wrote:
> > The C++ code looks good, but now that I think about this, I don't think this
> is
> > safe to do.
> > 
> > Even if jstring is the pointer to the string object (which also may not be
> > guaranteed... we have to check the docs), the JVM could compact and change
the
> > underlying memory address, even if the string is interned. So this map isn't
> > safe, since it could store stale addresses.
> > 
> > We could either make the map live on the Java side (since the key would be
the
> > String, it would continue to be consistent after compaction, etc) - but
would
> be
> > slower since an actual string compare will need to happen when using map
> > operations.
> > 
> > Or, we could keep this on the C++ side, but use a stable identifier for the
> > string, for example  System.identityHashCode(str) - which we could pass as
an
> > extra parameter to the JNI C++ code from the Java side.
> 
> Yeah, I suspected something like that. System.identityHashCode seems pretty
> reasonable.

OK, added System.identityHashCode to the latest version. A simple microbenchmark
variant of RecordHistogramTest (using several thousand iterations and 10
different histogram names) shows a cost per record improvement from 6.5micros to
1.3micros. Again, 6.5 micros won't break the bank, but every little bit helps on
Android =/.

[jdduke (slow), 2015-01-23 20:17:27]

jdduke@chromium.org changed reviewers:
+ nyquist@chromium.org

[jdduke (slow), 2015-01-23 20:17:28]

Oops, forgot to add nyquist@ per earlier post.

[Alexei Svitkine (slow), 2015-01-23 20:48:03]

LGTM!

[ppi, 2015-01-23 21:09:08]

lgtm too, thanks for doing this!

[jdduke (slow), 2015-01-26 18:57:41]

On 2015/01/23 21:09:08, ppi wrote:
> lgtm too, thanks for doing this!

nyquist@ anything else you'd like to see here?

[nyquist, 2015-01-26 22:20:45]

lgtm

[jdduke (slow), 2015-01-26 22:20:54]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-01-26 22:22:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/867063006/40001

[commit-bot: I haz the power, 2015-01-27 00:14:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-27 00:15:51]

Patchset 3 (id:??) landed as
https://crrev.com/825e324f2c26273ba8e60dba64e8b8739d0e3d85
Cr-Commit-Position: refs/heads/master@{#313174}