Rewrite Java histograms API implementation.

Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions.

Instead, this CL changes the code to do the caching on the Java
side, keeping pointers to the C++ Histogram objects in the cache.
This is safe to do because histogram objects are never freed, so the
pointers are guaranteed to stay valid.

BUG=535043
Committed: https://crrev.com/0f5bc5e0075fb1d3ca46bb109d6e4799b7b165e5
Cr-Commit-Position: refs/heads/master@{#385310}

[Alexei Svitkine (slow), 2016-03-29 21:00:59]

Patchset #3 (id:40001) has been deleted

[Alexei Svitkine (slow), 2016-03-29 21:01:07]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2016-03-29 21:01:45]

Description was changed from

==========
Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions. Instead, this CL changes
the code to do the caching on the Java side, keeping pointers to
the C++ Histogram objects in the cache. This is safe to do because
histogram objects are never freed, so the pointers are guaranteed
to stay valid.

BUG=
==========

to

==========
Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions.

Instead, this CL changes the code to do the caching on the Java
side, keeping pointers to the C++ Histogram objects in the cache.
This is safe to do because histogram objects are never freed, so the
pointers are guaranteed to stay valid.

BUG=535043
==========

[Alexei Svitkine (slow), 2016-03-29 21:01:51]

Patchset #3 (id:80001) has been deleted

[Alexei Svitkine (slow), 2016-03-29 21:01:56]

Patchset #2 (id:60001) has been deleted

[Alexei Svitkine (slow), 2016-03-29 21:02:02]

Patchset #2 (id:100001) has been deleted

[Alexei Svitkine (slow), 2016-03-29 21:02:08]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2016-03-29 21:43:18]

Patchset #1 (id:120001) has been deleted

[Alexei Svitkine (slow), 2016-03-30 15:14:48]

Patchset #2 (id:160001) has been deleted

[Alexei Svitkine (slow), 2016-03-30 16:55:37]

Patchset #1 (id:140001) has been deleted

[Alexei Svitkine (slow), 2016-03-30 21:46:44]

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

[Alexei Svitkine (slow), 2016-03-30 21:50:36]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-30 21:51:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828293002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828293002/180001

[commit-bot: I haz the power, 2016-03-30 23:02:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-30 23:02:34]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-03-31 22:27:08]

Friendly ping! For context, see the bug - the current implementation is not
correct per its assumptions about system.identityhashcode.

[nyquist, 2016-04-01 21:37:14]

thanks for writing a very thorough and explanatory CL description.

https://codereview.chromium.org/1828293002/diff/180001/base/android/java/src/...
File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java
(right):

https://codereview.chromium.org/1828293002/diff/180001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:36: private
static long getCachedHistogramKey(String name) {
Could you add a short comment about what returning 0 entails and how native
reacts to that here?

https://codereview.chromium.org/1828293002/diff/180001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:38: return
(key != null ? key : 0);
Nit: I personally think it reads better to use == instead of !=.

https://codereview.chromium.org/1828293002/diff/180001/base/android/record_hi...
File base/android/record_histogram.cc (right):

https://codereview.chromium.org/1828293002/diff/180001/base/android/record_hi...
base/android/record_histogram.cc:188: static HistogramBase*
HistogramFromKey(jlong j_histogram_key) {
I think this deserves a short comment for new readers about why this works. You
have a good explanation in your CL description.

[Alexei Svitkine (slow), 2016-04-04 15:08:09]

Comments addressed. PTAL.

https://codereview.chromium.org/1828293002/diff/180001/base/android/java/src/...
File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java
(right):

https://codereview.chromium.org/1828293002/diff/180001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:36: private
static long getCachedHistogramKey(String name) {
On 2016/04/01 21:37:14, nyquist wrote:
> Could you add a short comment about what returning 0 entails and how native
> reacts to that here?

Done.

https://codereview.chromium.org/1828293002/diff/180001/base/android/java/src/...
base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:38: return
(key != null ? key : 0);
On 2016/04/01 21:37:14, nyquist wrote:
> Nit: I personally think it reads better to use == instead of !=.

Done.

https://codereview.chromium.org/1828293002/diff/180001/base/android/record_hi...
File base/android/record_histogram.cc (right):

https://codereview.chromium.org/1828293002/diff/180001/base/android/record_hi...
base/android/record_histogram.cc:188: static HistogramBase*
HistogramFromKey(jlong j_histogram_key) {
On 2016/04/01 21:37:14, nyquist wrote:
> I think this deserves a short comment for new readers about why this works.
You
> have a good explanation in your CL description.

Done. Also expanded the description on the Java class.

[nyquist, 2016-04-05 21:25:21]

lgtm

[Alexei Svitkine (slow), 2016-04-05 21:26:53]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2016-04-05 21:27:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828293002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828293002/200001

[commit-bot: I haz the power, 2016-04-05 22:43:38]

Description was changed from

==========
Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions.

Instead, this CL changes the code to do the caching on the Java
side, keeping pointers to the C++ Histogram objects in the cache.
This is safe to do because histogram objects are never freed, so the
pointers are guaranteed to stay valid.

BUG=535043
==========

to

==========
Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions.

Instead, this CL changes the code to do the caching on the Java
side, keeping pointers to the C++ Histogram objects in the cache.
This is safe to do because histogram objects are never freed, so the
pointers are guaranteed to stay valid.

BUG=535043
==========

[commit-bot: I haz the power, 2016-04-05 22:43:39]

Committed patchset #2 (id:200001)

[commit-bot: I haz the power, 2016-04-05 22:44:38]

Description was changed from

==========
Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions.

Instead, this CL changes the code to do the caching on the Java
side, keeping pointers to the C++ Histogram objects in the cache.
This is safe to do because histogram objects are never freed, so the
pointers are guaranteed to stay valid.

BUG=535043
==========

to

==========
Rewrite Java histograms API implementation.

The previous version used System.identityHashCode() as a key to
cache histogram objects - but we've found that this is not safe
to do because there may be collisions.

Instead, this CL changes the code to do the caching on the Java
side, keeping pointers to the C++ Histogram objects in the cache.
This is safe to do because histogram objects are never freed, so the
pointers are guaranteed to stay valid.

BUG=535043

Committed: https://crrev.com/0f5bc5e0075fb1d3ca46bb109d6e4799b7b165e5
Cr-Commit-Position: refs/heads/master@{#385310}
==========

[commit-bot: I haz the power, 2016-04-05 22:44:39]

Patchset 2 (id:??) landed as
https://crrev.com/0f5bc5e0075fb1d3ca46bb109d6e4799b7b165e5
Cr-Commit-Position: refs/heads/master@{#385310}