Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
794273004:
Add a Java API for UMA histograms. (Closed)
DescriptionAdd a Java API for UMA histograms.
This patch adds a simple Java API for base/metrics, allowing to record
boolean and enumerated histograms directly from the Java code.
A test util (HistogramDelta) for histograms is also upstreamed from
Chrome for Android and used for testing the new API.
BUG=442300
Committed: https://crrev.com/dde405ee64e9cf1dab9bf38995acef27014de2f9
Cr-Commit-Position: refs/heads/master@{#312513}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address Ilya's comments. #Patch Set 3 : Update the GN build. #
Total comments: 8
Patch Set 4 : Rebase. #Patch Set 5 : Address Maria's comments. #
Total comments: 6
Patch Set 6 : Address Alexei's comments. #
Total comments: 3
Patch Set 7 : Rebase. #Patch Set 8 : Rebase over the cronet proguard config fix. #
Total comments: 3
Messages
Total messages: 42 (13 generated)
ppi@chromium.org changed reviewers: + asvitkine@chromium.org, mariakhomenko@chromium.org
Hi Alexei, Maria ptal. We will need a review or a stamp from a base/android OWNER, but I'm curious wdyt first.
ppi@chromium.org changed reviewers: + isherman@chromium.org
+Ilya as Alexei is ooo for two weeks. Ilya, could you take a look?
Seems like a reasonable thing to add :) https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:20: int sample = static_cast<int>(j_sample); Why int rather than bool? https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:24: ->Add(sample); nit: AddBoolean https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:46: base::android::ConvertJavaStringToUTF8(env, histogram_name).c_str()); nit: Do you need the .c_str() call? https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:47: if (histogram == NULL) { nit: Either "if (!histogram)" or "if (histogram == nullptr)" https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:54: } What is this used for? If it's for tests only, then please move it to a test-specific file. In C++, most people use //base/test/histogram_tester https://codereview.chromium.org/794273004/diff/1/base/android/record_histogram.h File base/android/record_histogram.h (right): https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.h:16: nit: Why this newline?
Thanks Ilya, ptal. https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:20: int sample = static_cast<int>(j_sample); On 2014/12/16 20:14:08, Ilya Sherman wrote: > Why int rather than bool? Done. https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:24: ->Add(sample); On 2014/12/16 20:14:08, Ilya Sherman wrote: > nit: AddBoolean Done. https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:46: base::android::ConvertJavaStringToUTF8(env, histogram_name).c_str()); On 2014/12/16 20:14:08, Ilya Sherman wrote: > nit: Do you need the .c_str() call? Done. https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:47: if (histogram == NULL) { On 2014/12/16 20:14:08, Ilya Sherman wrote: > nit: Either "if (!histogram)" or "if (histogram == nullptr)" Done. https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:54: } On 2014/12/16 20:14:08, Ilya Sherman wrote: > What is this used for? If it's for tests only, then please move it to a > test-specific file. In C++, most people use //base/test/histogram_tester Yes, this is needed for the Java-side test util being upstreamed in this same CL ( org.chromium.base.test.util.MetricsUtils.HistogramDelta ), and inspired by the predecessor of histogram_tester :). Unfortunately, we can't have test-specific native code backing Java test-specific utils - the context and more information is at http://crbug.com/415945 . In Chrome for Android, where this was originally added, we concluded that the gains from being able to test histograms in Java beat the cons of having a test-only method in the native code. Wdyt? https://codereview.chromium.org/794273004/diff/1/base/android/record_histogram.h File base/android/record_histogram.h (right): https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.h:16: On 2014/12/16 20:14:08, Ilya Sherman wrote: > nit: Why this newline? Done.
https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java (right): https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:20: * @param histogramName name of the histogram histogramName -> name nit: I believe there should be a period at the end of every param description here https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:30: * @param histogramName name of the histogram histogramName -> name https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:45: public static int getHistogramValueCountForTesting(String histogramName, int sample) { should this be name instead of histogramName for consistency with above? https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:53: nativeInitialize(); Should we be checking that this has been called inside of recordXXX?
LGTM, thanks. https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/1/base/android/record_histogra... base/android/record_histogram.cc:54: } On 2014/12/17 13:15:31, ppi wrote: > On 2014/12/16 20:14:08, Ilya Sherman wrote: > > What is this used for? If it's for tests only, then please move it to a > > test-specific file. In C++, most people use //base/test/histogram_tester > > Yes, this is needed for the Java-side test util being upstreamed in this same CL > ( org.chromium.base.test.util.MetricsUtils.HistogramDelta ), and inspired by the > predecessor of histogram_tester :). > > Unfortunately, we can't have test-specific native code backing Java > test-specific utils - the context and more information is at > http://crbug.com/415945 . > > In Chrome for Android, where this was originally added, we concluded that the > gains from being able to test histograms in Java beat the cons of having a > test-only method in the native code. > > Wdyt? Thanks for the explanation. I think that with the "ForTesting" suffix that you've added, this is now clear enough.
Thanks, Ilya and Maria! Maria, could you take another look? https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java (right): https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:20: * @param histogramName name of the histogram On 2014/12/17 19:49:29, Maria wrote: > histogramName -> name > > nit: I believe there should be a period at the end of every param description > here Done the rename. As for the @param, we're inconsistent on that:( and the style guide does not indicate one way or another. However, FWIW, both Oracle and our java-practices site lean towards no-capitalization, no-periods style for at-clauses (unless they are full sentence or multi-sentence). See http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html... https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:30: * @param histogramName name of the histogram On 2014/12/17 19:49:29, Maria wrote: > histogramName -> name Done. https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:45: public static int getHistogramValueCountForTesting(String histogramName, int sample) { On 2014/12/17 19:49:29, Maria wrote: > should this be name instead of histogramName for consistency with above? Done. https://codereview.chromium.org/794273004/diff/40001/base/android/java/src/or... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:53: nativeInitialize(); On 2014/12/17 19:49:29, Maria wrote: > Should we be checking that this has been called inside of recordXXX? Probably not - the system can be initialized from the native side as well (and this is usually the case).
lgtm
The CQ bit was checked by ppi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794273004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ppi@chromium.org changed reviewers: + jam@chromium.org
Thanks, Maria! Jam, could you stamp based on Ilya's and Maria's review?
lgtm % nits https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... base/android/record_histogram.cc:16: jclass, Nit: I thought Chromium style doesn't permit omitting param names. https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... base/android/record_histogram.cc:23: base::HistogramBase::kUmaTargetedHistogramFlag) Nit: This is already in base namespace right, so you don't need the base:: prefix here and elsewhere. https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... File base/android/record_histogram.h (right): https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... base/android/record_histogram.h:16: Nit: No empty line.
ppi@chromium.org changed reviewers: - jam@chromium.org
ppi@chromium.org changed reviewers: + thakis@chromium.org
Thanks, Alexei! Adding Nico as Jam is ooo. Nico, could you review this or stamp based on metrics OWNERs and Maria's review? https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... base/android/record_histogram.cc:16: jclass, On 2015/01/05 15:54:41, Alexei Svitkine wrote: > Nit: I thought Chromium style doesn't permit omitting param names. Done. https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... base/android/record_histogram.cc:23: base::HistogramBase::kUmaTargetedHistogramFlag) On 2015/01/05 15:54:41, Alexei Svitkine wrote: > Nit: This is already in base namespace right, so you don't need the base:: > prefix here and elsewhere. Done. https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... File base/android/record_histogram.h (right): https://codereview.chromium.org/794273004/diff/80001/base/android/record_hist... base/android/record_histogram.h:16: On 2015/01/05 15:54:41, Alexei Svitkine wrote: > Nit: No empty line. Done.
https://codereview.chromium.org/794273004/diff/100001/base/android/java/src/o... File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java (right): https://codereview.chromium.org/794273004/diff/100001/base/android/java/src/o... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:13: * these methods in performance-critical code. Isn't it a good thing that adding those to java code was slightly involved, then?
https://codereview.chromium.org/794273004/diff/100001/base/android/java/src/o... File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java (right): https://codereview.chromium.org/794273004/diff/100001/base/android/java/src/o... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:13: * these methods in performance-critical code. On 2015/01/08 18:32:11, Nico wrote: > Isn't it a good thing that adding those to java code was slightly involved, > then? If we ever see the JNI calls to histograms as a performance problem, we can optimize the Java API (add caching and flushing the records to native in batches as suggested on the bug by Maria), but first we need to have a Java API. We currently have hundreds of ad-hoc JNI plumbing functions for Java histograms in Chrome for Android and I don't recall any of them being called in a tight loop. Wdyt?
https://codereview.chromium.org/794273004/diff/100001/base/android/java/src/o... File base/android/java/src/org/chromium/base/metrics/RecordHistogram.java (right): https://codereview.chromium.org/794273004/diff/100001/base/android/java/src/o... base/android/java/src/org/chromium/base/metrics/RecordHistogram.java:13: * these methods in performance-critical code. On 2015/01/08 21:39:36, ppi wrote: > On 2015/01/08 18:32:11, Nico wrote: > > Isn't it a good thing that adding those to java code was slightly involved, > > then? > > If we ever see the JNI calls to histograms as a performance problem, we can > optimize the Java API (add caching and flushing the records to native in batches > as suggested on the bug by Maria), but first we need to have a Java API. We > currently have hundreds of ad-hoc JNI plumbing functions for Java histograms in > Chrome for Android and I don't recall any of them being called in a tight loop. > > Wdyt? Sounds fair. Build files lgtm.
Thanks!
The CQ bit was checked by ppi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794273004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
Update on this after chatting with cjhopman@ - the android_compile_rel bot fails on building cronet, as the proguard config for it is not yet taught not to strip the @VisibleForTesting methods. I'll try teaching it that in a separate CL.
Patchset #9 (id:160001) has been deleted
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by ppi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794273004/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/dde405ee64e9cf1dab9bf38995acef27014de2f9 Cr-Commit-Position: refs/heads/master@{#312513}
Message was sent while issue was closed.
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... base/android/record_histogram.cc:19: std::string histogram_name = ConvertJavaStringToUTF8(env, j_histogram_name); Ouch, JNI + string conversion isn't exactly the cheapest operation. Do we typically pass a Java string to native when recording UMA? Or is the string already embedded in the native code? We already have to modify the manifest with the string, would it be absurd to have a shared enum type that's used to convey the metric?
Message was sent while issue was closed.
https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... base/android/record_histogram.cc:19: std::string histogram_name = ConvertJavaStringToUTF8(env, j_histogram_name); On 2015/01/23 17:33:41, jdduke wrote: > Ouch, JNI + string conversion isn't exactly the cheapest operation. Do we > typically pass a Java string to native when recording UMA? Or is the string > already embedded in the native code? We already have to modify the manifest with > the string, would it be absurd to have a shared enum type that's used to convey > the metric? Having a shared enum adds more boiler plate that this change is trying to avoid. i.e. you'd need to update two extra files in addition to calling the API. Here's another idea: Have a map keyed by the Java string pointer that maps to the Histogram object. As a first try, look up the entry in this map. If found, done - and no string conversion was done in the process. If not found, use this code path (i.e. looking in the internal FactoryGet map). This way, in the common case, where a string literal from the Java side is passed (which is interned), it can just do a map look up based on the pointer without any conversion. You only need to do the conversion the first time that histogram is hit (where it's a cache miss) and in cases where the histogram name is not a string literal (i.e. if people concatenate histogram names - which shouldn't be a common use case).
Message was sent while issue was closed.
On 2015/01/23 17:33:41, jdduke wrote: > https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... > File base/android/record_histogram.cc (right): > > https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... > base/android/record_histogram.cc:19: std::string histogram_name = > ConvertJavaStringToUTF8(env, j_histogram_name); > Ouch, JNI + string conversion isn't exactly the cheapest operation Perf was discussed for a bit further up. The conclusion as I remember it was "if you're doing UMA stuff in perf-sensitive code, you're doing it wrong anyway, don't do that".
Message was sent while issue was closed.
On 2015/01/23 18:10:05, Nico wrote: > On 2015/01/23 17:33:41, jdduke wrote: > > > https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... > > File base/android/record_histogram.cc (right): > > > > > https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... > > base/android/record_histogram.cc:19: std::string histogram_name = > > ConvertJavaStringToUTF8(env, j_histogram_name); > > Ouch, JNI + string conversion isn't exactly the cheapest operation > > Perf was discussed for a bit further up. The conclusion as I remember it was "if > you're doing UMA stuff in perf-sensitive code, you're doing it wrong anyway, > don't do that". I trust developers as far as I can test them, and unfortunately this kind of overhead just accumulates as noise and likely won't show up in any profiles. i suppose we could trace each invocation, but there's no way we'd catch gradual adoption, even in perf-sensitive code.
Message was sent while issue was closed.
https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... File base/android/record_histogram.cc (right): https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... base/android/record_histogram.cc:19: std::string histogram_name = ConvertJavaStringToUTF8(env, j_histogram_name); On 2015/01/23 17:52:07, Alexei Svitkine wrote: > On 2015/01/23 17:33:41, jdduke wrote: > > Ouch, JNI + string conversion isn't exactly the cheapest operation. Do we > > typically pass a Java string to native when recording UMA? Or is the string > > already embedded in the native code? We already have to modify the manifest > with > > the string, would it be absurd to have a shared enum type that's used to > convey > > the metric? > > Having a shared enum adds more boiler plate that this change is trying to avoid. > i.e. you'd need to update two extra files in addition to calling the API. > > Here's another idea: > > Have a map keyed by the Java string pointer that maps to the Histogram object. > As a first try, look up the entry in this map. If found, done - and no string > conversion was done in the process. If not found, use this code path (i.e. > looking in the internal FactoryGet map). > > This way, in the common case, where a string literal from the Java side is > passed (which is interned), it can just do a map look up based on the pointer > without any conversion. > > You only need to do the conversion the first time that histogram is hit (where > it's a cache miss) and in cases where the histogram name is not a string literal > (i.e. if people concatenate histogram names - which shouldn't be a common use > case). That sounds pretty reasonable, any objections?
Message was sent while issue was closed.
On 2015/01/23 18:19:31, jdduke wrote: > https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... > File base/android/record_histogram.cc (right): > > https://codereview.chromium.org/794273004/diff/180001/base/android/record_his... > base/android/record_histogram.cc:19: std::string histogram_name = > ConvertJavaStringToUTF8(env, j_histogram_name); > On 2015/01/23 17:52:07, Alexei Svitkine wrote: > > On 2015/01/23 17:33:41, jdduke wrote: > > > Ouch, JNI + string conversion isn't exactly the cheapest operation. Do we > > > typically pass a Java string to native when recording UMA? Or is the string > > > already embedded in the native code? We already have to modify the manifest > > with > > > the string, would it be absurd to have a shared enum type that's used to > > convey > > > the metric? > > > > Having a shared enum adds more boiler plate that this change is trying to > avoid. > > i.e. you'd need to update two extra files in addition to calling the API. > > > > Here's another idea: > > > > Have a map keyed by the Java string pointer that maps to the Histogram object. > > As a first try, look up the entry in this map. If found, done - and no string > > conversion was done in the process. If not found, use this code path (i.e. > > looking in the internal FactoryGet map). > > > > This way, in the common case, where a string literal from the Java side is > > passed (which is interned), it can just do a map look up based on the pointer > > without any conversion. > > > > You only need to do the conversion the first time that histogram is hit (where > > it's a cache miss) and in cases where the histogram name is not a string > literal > > (i.e. if people concatenate histogram names - which shouldn't be a common use > > case). > > That sounds pretty reasonable, any objections? Posted a variant thereof here: https://codereview.chromium.org/867063006/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
