Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1887523002:
Record UMA for Physical Web state on debug actions (Closed)
DescriptionRecord UMA for Physical Web state on debug actions
The "Physical Web state" refers to:
* Data connection
* BT permission/enablement
* Location permission/provider
We record this state:
* When launching the ListUrlsActivity from the diagnostics page
* When launching the ListUrlsActivity from the preferences page
BUG=594194
Committed: https://crrev.com/f4766630a78c1e35ef85211da0829f54d85541df
Cr-Commit-Position: refs/heads/master@{#388001}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 22 (5 generated)
cco3@chromium.org changed reviewers: + mattreynolds@chromium.org
lgtm
cco3@chromium.org changed reviewers: + yfriedman@chromium.org
This is (probably) a safe part of a recently reverted commit. If it's not, we'll find out later when specific on-device memory tests are run, post commit. https://codereview.chromium.org/1878063002/
https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: handleEnum(context, createStateString(LOCATION_SERVICES, actionName), you need to add the new histograms to histograms.xml https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: public void run() { wow! I just saw the impl of uploadActions and this is pretty inefficient use of sequential JNI calls. Do any of these actions get hit frequently because I'm wondering whether the java abstractions are adding extra overhead and whether it's worth custom JNI calls. Can you grab a quick trace of this function?
helenquin1224@gmail.com changed reviewers: + helenquin1224@gmail.com
lgtm
https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: handleEnum(context, createStateString(LOCATION_SERVICES, actionName), On 2016/04/14 02:28:46, Yaron wrote: > you need to add the new histograms to histograms.xml They are already there. I didn't take them out with the revert. https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: public void run() { On 2016/04/14 02:28:46, Yaron wrote: > wow! I just saw the impl of uploadActions and this is pretty inefficient use of > sequential JNI calls. Do any of these actions get hit frequently because I'm > wondering whether the java abstractions are adding extra overhead and whether > it's worth custom JNI calls. Can you grab a quick trace of this function? I'll look into it.
On 2016/04/15 22:12:57, cco3 wrote: > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > (right): > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > handleEnum(context, createStateString(LOCATION_SERVICES, actionName), > On 2016/04/14 02:28:46, Yaron wrote: > > you need to add the new histograms to histograms.xml > > They are already there. I didn't take them out with the revert. > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > public void run() { > On 2016/04/14 02:28:46, Yaron wrote: > > wow! I just saw the impl of uploadActions and this is pretty inefficient use > of > > sequential JNI calls. Do any of these actions get hit frequently because I'm > > wondering whether the java abstractions are adding extra overhead and whether > > it's worth custom JNI calls. Can you grab a quick trace of this function? > > I'll look into it. I've added a trace file to the bug. In case it's not clear, this sequential call of upload is called on startup (so I'm not sure why it would matter how frequently the actions get hit).
On 2016/04/15 23:57:38, cco3 wrote: > On 2016/04/15 22:12:57, cco3 wrote: > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > (right): > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > handleEnum(context, createStateString(LOCATION_SERVICES, actionName), > > On 2016/04/14 02:28:46, Yaron wrote: > > > you need to add the new histograms to histograms.xml > > > > They are already there. I didn't take them out with the revert. > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > public void run() { > > On 2016/04/14 02:28:46, Yaron wrote: > > > wow! I just saw the impl of uploadActions and this is pretty inefficient use > > of > > > sequential JNI calls. Do any of these actions get hit frequently because I'm > > > wondering whether the java abstractions are adding extra overhead and > whether > > > it's worth custom JNI calls. Can you grab a quick trace of this function? > > > > I'll look into it. > > I've added a trace file to the bug. In case it's not clear, this sequential > call of upload is called on startup (so I'm not sure why it would matter how > frequently the actions get hit). The trace failed to load for me. Ok, I hadn't looked too deeply but I thought that these could build up for some period of time and then eventually you catch up. If you had a synchronous block with lots of JNI calls, it could be quite inefficient. Since you're only caching these until deferred startup, it shouldn't be too bad. That said, I wonder how much data you'd lose if you only captured once native was loaded.
On 2016/04/18 18:43:19, Yaron wrote: > On 2016/04/15 23:57:38, cco3 wrote: > > On 2016/04/15 22:12:57, cco3 wrote: > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > > handleEnum(context, createStateString(LOCATION_SERVICES, actionName), > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > you need to add the new histograms to histograms.xml > > > > > > They are already there. I didn't take them out with the revert. > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > > public void run() { > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > wow! I just saw the impl of uploadActions and this is pretty inefficient > use > > > of > > > > sequential JNI calls. Do any of these actions get hit frequently because > I'm > > > > wondering whether the java abstractions are adding extra overhead and > > whether > > > > it's worth custom JNI calls. Can you grab a quick trace of this function? > > > > > > I'll look into it. > > > > I've added a trace file to the bug. In case it's not clear, this sequential > > call of upload is called on startup (so I'm not sure why it would matter how > > frequently the actions get hit). > > The trace failed to load for me. What tool were you using? dmtracedump works for me. > Ok, I hadn't looked too deeply but I thought that these could build up for some > period of time and then eventually you catch up. If you had a synchronous block > with lots of JNI calls, it could be quite inefficient. Since you're only caching > these until deferred startup, it shouldn't be too bad. That said, I wonder how > much data you'd lose if you only captured once native was loaded. Probably quite a lot, especially on low-memory devices where chrome gets evicted often.
On 2016/04/18 18:51:22, cco3 wrote: > On 2016/04/18 18:43:19, Yaron wrote: > > On 2016/04/15 23:57:38, cco3 wrote: > > > On 2016/04/15 22:12:57, cco3 wrote: > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > File > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > > > handleEnum(context, createStateString(LOCATION_SERVICES, actionName), > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > you need to add the new histograms to histograms.xml > > > > > > > > They are already there. I didn't take them out with the revert. > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > > > public void run() { > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > wow! I just saw the impl of uploadActions and this is pretty inefficient > > use > > > > of > > > > > sequential JNI calls. Do any of these actions get hit frequently because > > I'm > > > > > wondering whether the java abstractions are adding extra overhead and > > > whether > > > > > it's worth custom JNI calls. Can you grab a quick trace of this > function? > > > > > > > > I'll look into it. > > > > > > I've added a trace file to the bug. In case it's not clear, this sequential > > > call of upload is called on startup (so I'm not sure why it would matter how > > > frequently the actions get hit). > > > > The trace failed to load for me. > > What tool were you using? dmtracedump works for me. > > > Ok, I hadn't looked too deeply but I thought that these could build up for > some > > period of time and then eventually you catch up. If you had a synchronous > block > > with lots of JNI calls, it could be quite inefficient. Since you're only > caching > > these until deferred startup, it shouldn't be too bad. That said, I wonder how > > much data you'd lose if you only captured once native was loaded. > > Probably quite a lot, especially on low-memory devices where chrome gets evicted > often. Oh, I was trying chrome's about:tracing, looking now. You're saying that most of your actions happen before the first page is loaded?
On 2016/04/18 18:56:58, Yaron wrote: > On 2016/04/18 18:51:22, cco3 wrote: > > On 2016/04/18 18:43:19, Yaron wrote: > > > On 2016/04/15 23:57:38, cco3 wrote: > > > > On 2016/04/15 22:12:57, cco3 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > File > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > > > > handleEnum(context, createStateString(LOCATION_SERVICES, actionName), > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > you need to add the new histograms to histograms.xml > > > > > > > > > > They are already there. I didn't take them out with the revert. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > > > > public void run() { > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > wow! I just saw the impl of uploadActions and this is pretty > inefficient > > > use > > > > > of > > > > > > sequential JNI calls. Do any of these actions get hit frequently > because > > > I'm > > > > > > wondering whether the java abstractions are adding extra overhead and > > > > whether > > > > > > it's worth custom JNI calls. Can you grab a quick trace of this > > function? > > > > > > > > > > I'll look into it. > > > > > > > > I've added a trace file to the bug. In case it's not clear, this > sequential > > > > call of upload is called on startup (so I'm not sure why it would matter > how > > > > frequently the actions get hit). > > > > > > The trace failed to load for me. > > > > What tool were you using? dmtracedump works for me. > > > > > Ok, I hadn't looked too deeply but I thought that these could build up for > > some > > > period of time and then eventually you catch up. If you had a synchronous > > block > > > with lots of JNI calls, it could be quite inefficient. Since you're only > > caching > > > these until deferred startup, it shouldn't be too bad. That said, I wonder > how > > > much data you'd lose if you only captured once native was loaded. > > > > Probably quite a lot, especially on low-memory devices where chrome gets > evicted > > often. > > Oh, I was trying chrome's about:tracing, looking now. > You're saying that most of your actions happen before the first page is loaded? No, I'm saying that our actions happen apart from necessary intent to even browse. We have a service that fires a notification. This can happen regardless of whether or not the native library is loaded. Users can tap on the notification and look at a list of URLs. Before they select a URL, we don't use the native library at all.
On 2016/04/18 19:02:27, cco3 wrote: > On 2016/04/18 18:56:58, Yaron wrote: > > On 2016/04/18 18:51:22, cco3 wrote: > > > On 2016/04/18 18:43:19, Yaron wrote: > > > > On 2016/04/15 23:57:38, cco3 wrote: > > > > > On 2016/04/15 22:12:57, cco3 wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > File > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > > > > > handleEnum(context, createStateString(LOCATION_SERVICES, actionName), > > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > > you need to add the new histograms to histograms.xml > > > > > > > > > > > > They are already there. I didn't take them out with the revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > > > > > public void run() { > > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > > wow! I just saw the impl of uploadActions and this is pretty > > inefficient > > > > use > > > > > > of > > > > > > > sequential JNI calls. Do any of these actions get hit frequently > > because > > > > I'm > > > > > > > wondering whether the java abstractions are adding extra overhead > and > > > > > whether > > > > > > > it's worth custom JNI calls. Can you grab a quick trace of this > > > function? > > > > > > > > > > > > I'll look into it. > > > > > > > > > > I've added a trace file to the bug. In case it's not clear, this > > sequential > > > > > call of upload is called on startup (so I'm not sure why it would matter > > how > > > > > frequently the actions get hit). > > > > > > > > The trace failed to load for me. > > > > > > What tool were you using? dmtracedump works for me. > > > > > > > Ok, I hadn't looked too deeply but I thought that these could build up for > > > some > > > > period of time and then eventually you catch up. If you had a synchronous > > > block > > > > with lots of JNI calls, it could be quite inefficient. Since you're only > > > caching > > > > these until deferred startup, it shouldn't be too bad. That said, I wonder > > how > > > > much data you'd lose if you only captured once native was loaded. > > > > > > Probably quite a lot, especially on low-memory devices where chrome gets > > evicted > > > often. > > > > Oh, I was trying chrome's about:tracing, looking now. > > You're saying that most of your actions happen before the first page is > loaded? > > No, I'm saying that our actions happen apart from necessary intent to even > browse. We have a service that fires a notification. This can happen > regardless of whether or not the native library is loaded. Users can tap on the > notification and look at a list of URLs. Before they select a URL, we don't use > the native library at all. Ah, ok. Thanks for your patience and for clarifying. Ok, so as long as things are based on user actions, this seems like it's unlikely to matter in practice. Scanning the list, there are ones about notifications but perhaps not too common since presumably they don't bubble up to notification status too often? I do see PWS_BACKGROUND_RESOLVE_TIMES and without knowing the details of the protocol looks like it gets hit for every url you come across. Is it per beacon you see based on background scanning when you walk in/out of range? Truthfully from a perf POV it probably wouldn't matter. I do question somewhat the utility of capturing this metric though, or at least as thoroughly and could probably be sampled. Somewhat orthogonal so this change and at least from the trace looks like this is dominated by the shared pref removal so lgtm
On 2016/04/18 19:24:45, Yaron wrote: > On 2016/04/18 19:02:27, cco3 wrote: > > On 2016/04/18 18:56:58, Yaron wrote: > > > On 2016/04/18 18:51:22, cco3 wrote: > > > > On 2016/04/18 18:43:19, Yaron wrote: > > > > > On 2016/04/15 23:57:38, cco3 wrote: > > > > > > On 2016/04/15 22:12:57, cco3 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > File > > > > > > > > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > > > > > > handleEnum(context, createStateString(LOCATION_SERVICES, > actionName), > > > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > > > you need to add the new histograms to histograms.xml > > > > > > > > > > > > > > They are already there. I didn't take them out with the revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > > > > > > public void run() { > > > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > > > wow! I just saw the impl of uploadActions and this is pretty > > > inefficient > > > > > use > > > > > > > of > > > > > > > > sequential JNI calls. Do any of these actions get hit frequently > > > because > > > > > I'm > > > > > > > > wondering whether the java abstractions are adding extra overhead > > and > > > > > > whether > > > > > > > > it's worth custom JNI calls. Can you grab a quick trace of this > > > > function? > > > > > > > > > > > > > > I'll look into it. > > > > > > > > > > > > I've added a trace file to the bug. In case it's not clear, this > > > sequential > > > > > > call of upload is called on startup (so I'm not sure why it would > matter > > > how > > > > > > frequently the actions get hit). > > > > > > > > > > The trace failed to load for me. > > > > > > > > What tool were you using? dmtracedump works for me. > > > > > > > > > Ok, I hadn't looked too deeply but I thought that these could build up > for > > > > some > > > > > period of time and then eventually you catch up. If you had a > synchronous > > > > block > > > > > with lots of JNI calls, it could be quite inefficient. Since you're only > > > > caching > > > > > these until deferred startup, it shouldn't be too bad. That said, I > wonder > > > how > > > > > much data you'd lose if you only captured once native was loaded. > > > > > > > > Probably quite a lot, especially on low-memory devices where chrome gets > > > evicted > > > > often. > > > > > > Oh, I was trying chrome's about:tracing, looking now. > > > You're saying that most of your actions happen before the first page is > > loaded? > > > > No, I'm saying that our actions happen apart from necessary intent to even > > browse. We have a service that fires a notification. This can happen > > regardless of whether or not the native library is loaded. Users can tap on > the > > notification and look at a list of URLs. Before they select a URL, we don't > use > > the native library at all. > > Ah, ok. Thanks for your patience and for clarifying. Ok, so as long as things np, thanks for your help. > are based on user actions, this seems like it's unlikely to matter in practice. > Scanning the list, there are ones about notifications but perhaps not too common > since presumably they don't bubble up to notification status too often? I do see Most notification counts are for notification presses, not notification firing. We do record when opt-in notifications fire, but those are at max twice per user, and we intend to move to once per user. > PWS_BACKGROUND_RESOLVE_TIMES and without knowing the details of the protocol > looks like it gets hit for every url you come across. Is it per beacon you see > based on background scanning when you walk in/out of range? Yes, currently this is on every time we come within range of a beacon. > > Truthfully from a perf POV it probably wouldn't matter. I do question somewhat > the utility of capturing this metric though, or at least as thoroughly and could > probably be sampled. I think we'd be fine with that if you have a suggestion for how we would implement the sampling. Just randomly send 1/10 of the values? > > Somewhat orthogonal so this change and at least from the trace looks like this > is dominated by the shared pref removal so lgtm
The CQ bit was checked by cco3@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887523002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887523002/1
On 2016/04/18 19:33:39, cco3 wrote: > On 2016/04/18 19:24:45, Yaron wrote: > > On 2016/04/18 19:02:27, cco3 wrote: > > > On 2016/04/18 18:56:58, Yaron wrote: > > > > On 2016/04/18 18:51:22, cco3 wrote: > > > > > On 2016/04/18 18:43:19, Yaron wrote: > > > > > > On 2016/04/15 23:57:38, cco3 wrote: > > > > > > > On 2016/04/15 22:12:57, cco3 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > File > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:219: > > > > > > > > handleEnum(context, createStateString(LOCATION_SERVICES, > > actionName), > > > > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > > > > you need to add the new histograms to histograms.xml > > > > > > > > > > > > > > > > They are already there. I didn't take them out with the revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1887523002/diff/1/chrome/android/java/src/org... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:315: > > > > > > > > public void run() { > > > > > > > > On 2016/04/14 02:28:46, Yaron wrote: > > > > > > > > > wow! I just saw the impl of uploadActions and this is pretty > > > > inefficient > > > > > > use > > > > > > > > of > > > > > > > > > sequential JNI calls. Do any of these actions get hit frequently > > > > because > > > > > > I'm > > > > > > > > > wondering whether the java abstractions are adding extra > overhead > > > and > > > > > > > whether > > > > > > > > > it's worth custom JNI calls. Can you grab a quick trace of this > > > > > function? > > > > > > > > > > > > > > > > I'll look into it. > > > > > > > > > > > > > > I've added a trace file to the bug. In case it's not clear, this > > > > sequential > > > > > > > call of upload is called on startup (so I'm not sure why it would > > matter > > > > how > > > > > > > frequently the actions get hit). > > > > > > > > > > > > The trace failed to load for me. > > > > > > > > > > What tool were you using? dmtracedump works for me. > > > > > > > > > > > Ok, I hadn't looked too deeply but I thought that these could build up > > for > > > > > some > > > > > > period of time and then eventually you catch up. If you had a > > synchronous > > > > > block > > > > > > with lots of JNI calls, it could be quite inefficient. Since you're > only > > > > > caching > > > > > > these until deferred startup, it shouldn't be too bad. That said, I > > wonder > > > > how > > > > > > much data you'd lose if you only captured once native was loaded. > > > > > > > > > > Probably quite a lot, especially on low-memory devices where chrome gets > > > > evicted > > > > > often. > > > > > > > > Oh, I was trying chrome's about:tracing, looking now. > > > > You're saying that most of your actions happen before the first page is > > > loaded? > > > > > > No, I'm saying that our actions happen apart from necessary intent to even > > > browse. We have a service that fires a notification. This can happen > > > regardless of whether or not the native library is loaded. Users can tap on > > the > > > notification and look at a list of URLs. Before they select a URL, we don't > > use > > > the native library at all. > > > > Ah, ok. Thanks for your patience and for clarifying. Ok, so as long as things > > np, thanks for your help. > > > are based on user actions, this seems like it's unlikely to matter in > practice. > > Scanning the list, there are ones about notifications but perhaps not too > common > > since presumably they don't bubble up to notification status too often? I do > see > > Most notification counts are for notification presses, not notification firing. > We do record when opt-in notifications fire, but those are at max twice per > user, and we intend to move to once per user. > > > PWS_BACKGROUND_RESOLVE_TIMES and without knowing the details of the protocol > > looks like it gets hit for every url you come across. Is it per beacon you see > > based on background scanning when you walk in/out of range? > > Yes, currently this is on every time we come within range of a beacon. > > > > > Truthfully from a perf POV it probably wouldn't matter. I do question somewhat > > the utility of capturing this metric though, or at least as thoroughly and > could > > probably be sampled. > > I think we'd be fine with that if you have a suggestion for how we would > implement the sampling. Just randomly send 1/10 of the values? Ya, I didn't have anything too fancy in mind. I thought briefly about ensuring we get some data in sparsely populated areas but I suspect it's not critical that each user actually reports this metric and in aggregate you will still get some visibility so naive 1/10 seems fine > > > > > Somewhat orthogonal so this change and at least from the trace looks like this > > is dominated by the shared pref removal so lgtm
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Record UMA for Physical Web state on debug actions The "Physical Web state" refers to: * Data connection * BT permission/enablement * Location permission/provider We record this state: * When launching the ListUrlsActivity from the diagnostics page * When launching the ListUrlsActivity from the preferences page BUG=594194 ========== to ========== Record UMA for Physical Web state on debug actions The "Physical Web state" refers to: * Data connection * BT permission/enablement * Location permission/provider We record this state: * When launching the ListUrlsActivity from the diagnostics page * When launching the ListUrlsActivity from the preferences page BUG=594194 Committed: https://crrev.com/f4766630a78c1e35ef85211da0829f54d85541df Cr-Commit-Position: refs/heads/master@{#388001} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f4766630a78c1e35ef85211da0829f54d85541df Cr-Commit-Position: refs/heads/master@{#388001} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
