Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2015533005:
Record the return value of NativeLibraryPreloader.loadLibrary in UMA (Closed)
|
Created:
4 years, 7 months ago by michaelbai Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord the return value of NativeLibraryPreloader.loadLibrary in UMA
BUG=614166
Committed: https://crrev.com/8369bdb2f158539fae0dc33eff0141394be164e6
Cr-Commit-Position: refs/heads/master@{#396632}
Patch Set 1 #Patch Set 2 : add histogram.xml #
Total comments: 9
Patch Set 3 : address comments #
Total comments: 5
Patch Set 4 : address comments #Patch Set 5 : #
Messages
Total messages: 33 (12 generated)
Description was changed from ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA BUG=614166 ========== to ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA The return value is defined by NativeLibraryPreloader implementation, can't be defined as emum in chromium, so record integer value is good enough. BUG=614166 ==========
michaelbai@chromium.org changed reviewers: + asvitkine@chromium.org, torne@chromium.org
https://codereview.chromium.org/2015533005/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2015533005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:5111: +<histogram name="ChromiumAndroidNativeLibraryPreloader.BrowserStates"> torne@ We can define the enum for current return value, let me if you want to.
On 2016/05/26 04:33:28, michaelbai wrote: > https://codereview.chromium.org/2015533005/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2015533005/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:5111: +<histogram > name="ChromiumAndroidNativeLibraryPreloader.BrowserStates"> > torne@ We can define the enum for current return value, let me if you want to. If we can define the enum for the current known values without preventing future added values from being recorded, then yes, we should do that.
https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:91: private int mLibraryPreloaderStatus; Initialise this to -1 or something? It will get default-initialised to 0, which if we somehow end up uploading the value without having set it will look like a success.
https://codereview.chromium.org/2015533005/diff/20001/base/android/library_lo... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/2015533005/diff/20001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:146: "ChromiumAndroidNativeLibraryPreloader.RendererStates", I suggest not adding a new histogram prefix unless you expect it will be used a lot for new metrics. So perhaps you can nest these under the existing Android.* prefix? e.g. Android.NativeLibraryPreloader.RendererStates Also, this is the result of loadLibrary(), right? So maybe Android.NativeLibraryPreloader.Result.Browser and Android.NativeLibraryPreloader.Result.Renderer? The "Browser" and "Renderer" suffixes are a standard way on how to permute a histogram in UMA and is supported via histogram_suffixes in histograms.xml. https://codereview.chromium.org/2015533005/diff/20001/content/renderer/render... File content/renderer/renderer_main.cc (right): https://codereview.chromium.org/2015533005/diff/20001/content/renderer/render... content/renderer/renderer_main.cc:153: base::android::RecordChromiumAndroidLibraryLoaderRendererHistogram(); Not sure what "Chromium" and "Android" add to the name of this function. It's already in android namespace and this is the Chromium project. Maybe simplify it?
PTAL https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:91: private int mLibraryPreloaderStatus; On 2016/05/26 10:53:24, Torne wrote: > Initialise this to -1 or something? It will get default-initialised to 0, which > if we somehow end up uploading the value without having set it will look like a > success. We don't need to do that, first, if so, this will assume the method never return -1, second, this value is only recorded if NativeLibraryPreloader is set, and we also have boolean to indicate whether the result is registered for renderer in native side. https://codereview.chromium.org/2015533005/diff/20001/base/android/library_lo... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/2015533005/diff/20001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:146: "ChromiumAndroidNativeLibraryPreloader.RendererStates", On 2016/05/26 14:56:29, Alexei Svitkine (slow) wrote: > I suggest not adding a new histogram prefix unless you expect it will be used a > lot for new metrics. > > So perhaps you can nest these under the existing Android.* prefix? > > e.g. Android.NativeLibraryPreloader.RendererStates > > Also, this is the result of loadLibrary(), right? > > So maybe Android.NativeLibraryPreloader.Result.Browser and > Android.NativeLibraryPreloader.Result.Renderer? > > The "Browser" and "Renderer" suffixes are a standard way on how to permute a > histogram in UMA and is supported via histogram_suffixes in histograms.xml. Done. https://codereview.chromium.org/2015533005/diff/20001/content/renderer/render... File content/renderer/renderer_main.cc (right): https://codereview.chromium.org/2015533005/diff/20001/content/renderer/render... content/renderer/renderer_main.cc:153: base::android::RecordChromiumAndroidLibraryLoaderRendererHistogram(); On 2016/05/26 14:56:29, Alexei Svitkine (slow) wrote: > Not sure what "Chromium" and "Android" add to the name of this function. It's > already in android namespace and this is the Chromium project. Maybe simplify > it? You are right, we could have simple name here
lgtm % comments https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:82: static void RecordChromiumAndroidLinkerRendererHistogram() { If you only call it locally in the file, put it in the anon namespace above instead. Also, seems like you can simplify this function name as well. https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:143: static void RecordLibraryPreloaderRendereHistogram() { Put this in the anon namespace above. https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:151: void RecordLibraryLoaderRendererHistogram() { This is recording multiple histograms right? Make the name plural. https://codereview.chromium.org/2015533005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2015533005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:411: + The return value of NativeLibraryPreloader.loadLibrary() in browser process. Is this loaded once per browser process start or multiple times? Mention here. Same comment for the one below.
lgtm % comments https://codereview.chromium.org/2015533005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2015533005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:411: + The return value of NativeLibraryPreloader.loadLibrary() in browser process. Is this loaded once per browser process start or multiple times? Mention here. Same comment for the one below.
https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:82: static void RecordChromiumAndroidLinkerRendererHistogram() { On 2016/05/26 19:14:11, Alexei Svitkine (slow) wrote: > If you only call it locally in the file, put it in the anon namespace above > instead. Also, seems like you can simplify this function name as well. Moved to anon namespace, but the name is correct. https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:143: static void RecordLibraryPreloaderRendereHistogram() { On 2016/05/26 19:14:11, Alexei Svitkine (slow) wrote: > Put this in the anon namespace above. Moved to anon namespace. https://codereview.chromium.org/2015533005/diff/40001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:151: void RecordLibraryLoaderRendererHistogram() { On 2016/05/26 19:14:11, Alexei Svitkine (slow) wrote: > This is recording multiple histograms right? Make the name plural. Done. https://codereview.chromium.org/2015533005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2015533005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:411: + The return value of NativeLibraryPreloader.loadLibrary() in browser process. On 2016/05/26 19:14:11, Alexei Svitkine (slow) wrote: > Is this loaded once per browser process start or multiple times? Mention here. > Same comment for the one below. Done.
Description was changed from ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA The return value is defined by NativeLibraryPreloader implementation, can't be defined as emum in chromium, so record integer value is good enough. BUG=614166 ========== to ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA BUG=614166 ==========
https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:91: private int mLibraryPreloaderStatus; On 2016/05/26 19:10:35, michaelbai wrote: > On 2016/05/26 10:53:24, Torne wrote: > > Initialise this to -1 or something? It will get default-initialised to 0, > which > > if we somehow end up uploading the value without having set it will look like > a > > success. > > We don't need to do that, first, if so, this will assume the method never return > -1 It won't, it's an errno-style error code, they're conventionally positive numbers for errors (also we define the codes, so we can just not do that). Even if it does, better to confuse it with an error value than with the success value. > second, this value is only recorded if NativeLibraryPreloader is set, and we > also have boolean to indicate whether the result is registered for renderer in > native side. Right, it's not intended that it will upload an uninitialised value, it would be a bug. What I'm saying is that if the "uninitialised" value is "success" then we'd never *notice* if there was a bug since the stats would look like everything was fine, and so it would be safer to initialise this to a "failure" value.
https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:91: private int mLibraryPreloaderStatus; On 2016/05/27 10:21:13, Torne wrote: > On 2016/05/26 19:10:35, michaelbai wrote: > > On 2016/05/26 10:53:24, Torne wrote: > > > Initialise this to -1 or something? It will get default-initialised to 0, > > which > > > if we somehow end up uploading the value without having set it will look > like > > a > > > success. > > > > We don't need to do that, first, if so, this will assume the method never > return > > -1 > > It won't, it's an errno-style error code, they're conventionally positive > numbers for errors (also we define the codes, so we can just not do that). Even > if it does, better to confuse it with an error value than with the success > value. > > > second, this value is only recorded if NativeLibraryPreloader is set, and we > > also have boolean to indicate whether the result is registered for renderer in > > native side. > > Right, it's not intended that it will upload an uninitialised value, it would be > a bug. What I'm saying is that if the "uninitialised" value is "success" then > we'd never *notice* if there was a bug since the stats would look like > everything was fine, and so it would be safer to initialise this to a "failure" > value. I probably miss something here, I meant current implementation doesn't assume anything of return value, would it be better than relying on return value can't be -1?
On 2016/05/27 15:52:42, michaelbai wrote: > https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... > File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java > (right): > > https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... > base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:91: > private int mLibraryPreloaderStatus; > On 2016/05/27 10:21:13, Torne wrote: > > On 2016/05/26 19:10:35, michaelbai wrote: > > > On 2016/05/26 10:53:24, Torne wrote: > > > > Initialise this to -1 or something? It will get default-initialised to 0, > > > which > > > > if we somehow end up uploading the value without having set it will look > > like > > > a > > > > success. > > > > > > We don't need to do that, first, if so, this will assume the method never > > return > > > -1 > > > > It won't, it's an errno-style error code, they're conventionally positive > > numbers for errors (also we define the codes, so we can just not do that). > Even > > if it does, better to confuse it with an error value than with the success > > value. > > > > > second, this value is only recorded if NativeLibraryPreloader is set, and we > > > also have boolean to indicate whether the result is registered for renderer > in > > > native side. > > > > Right, it's not intended that it will upload an uninitialised value, it would > be > > a bug. What I'm saying is that if the "uninitialised" value is "success" then > > we'd never *notice* if there was a bug since the stats would look like > > everything was fine, and so it would be safer to initialise this to a > "failure" > > value. > > I probably miss something here, I meant current implementation doesn't assume > anything of return value, would it be better than relying on return value can't > be -1? It doesn't really matter whether the return value can be -1 or not. What matters is initialising it to a value that doesn't mean "everything is fine", so that if the UMA dashboard ends up showing us a bunch of clients where everything is not fine, we will actually investigate. If, by some unlikely sequence of events, -1 has actually become a legitimate error code, then we may be confused for a bit and misinterpret this data, but we will still have reason to investigate. Also, we *do* control these values. We won't use -1 in the framework code.
On 2016/05/27 16:02:31, Torne wrote: > On 2016/05/27 15:52:42, michaelbai wrote: > > > https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... > > File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java > > (right): > > > > > https://codereview.chromium.org/2015533005/diff/20001/base/android/java/src/o... > > base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:91: > > private int mLibraryPreloaderStatus; > > On 2016/05/27 10:21:13, Torne wrote: > > > On 2016/05/26 19:10:35, michaelbai wrote: > > > > On 2016/05/26 10:53:24, Torne wrote: > > > > > Initialise this to -1 or something? It will get default-initialised to > 0, > > > > which > > > > > if we somehow end up uploading the value without having set it will look > > > like > > > > a > > > > > success. > > > > > > > > We don't need to do that, first, if so, this will assume the method never > > > return > > > > -1 > > > > > > It won't, it's an errno-style error code, they're conventionally positive > > > numbers for errors (also we define the codes, so we can just not do that). > > Even > > > if it does, better to confuse it with an error value than with the success > > > value. > > > > > > > second, this value is only recorded if NativeLibraryPreloader is set, and > we > > > > also have boolean to indicate whether the result is registered for > renderer > > in > > > > native side. > > > > > > Right, it's not intended that it will upload an uninitialised value, it > would > > be > > > a bug. What I'm saying is that if the "uninitialised" value is "success" > then > > > we'd never *notice* if there was a bug since the stats would look like > > > everything was fine, and so it would be safer to initialise this to a > > "failure" > > > value. > > > > I probably miss something here, I meant current implementation doesn't assume > > anything of return value, would it be better than relying on return value > can't > > be -1? > > It doesn't really matter whether the return value can be -1 or not. What matters > is initialising it to a value that doesn't mean "everything is fine", so that if > the UMA dashboard ends up showing us a bunch of clients where everything is not > fine, we will actually investigate. If, by some unlikely sequence of events, -1 > has actually become a legitimate error code, then we may be confused for a bit > and misinterpret this data, but we will still have reason to investigate. > > Also, we *do* control these values. We won't use -1 in the framework code. Aha, I see, this make sense now.
Initialize to -1, PTAL
LGTM, thanks!
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2015533005/#ps80001 (title: " ")
The CQ bit was unchecked by michaelbai@chromium.org
The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run
michaelbai@chromium.org changed reviewers: + avi@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015533005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015533005/80001
avi@ for content/renderer
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015533005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015533005/80001
Message was sent while issue was closed.
Description was changed from ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA BUG=614166 ========== to ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA BUG=614166 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA BUG=614166 ========== to ========== Record the return value of NativeLibraryPreloader.loadLibrary in UMA BUG=614166 Committed: https://crrev.com/8369bdb2f158539fae0dc33eff0141394be164e6 Cr-Commit-Position: refs/heads/master@{#396632} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8369bdb2f158539fae0dc33eff0141394be164e6 Cr-Commit-Position: refs/heads/master@{#396632} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
