Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1994403002:
Mark trace data when launching WebView (Closed)
|
Created:
4 years, 7 months ago by Yoland Yan(Google) Modified:
4 years, 6 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org, sgurun-gerrit only Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMark trace data when launching WebView
BUG=613419
Committed: https://crrev.com/a9fa54ee41fb8ef334883c808123268bea188003
Cr-Commit-Position: refs/heads/master@{#396905}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Change #
Total comments: 8
Patch Set 3 : helper functions #
Total comments: 9
Patch Set 4 : Hardcode default tracing tag #Messages
Total messages: 30 (9 generated)
yolandyan@google.com changed reviewers: + alexandermont@chromium.org, timvolodine@chromium.org, torne@chromium.org
Does the webview end up with the correct size just setting it as the content view directly like this? (i.e. is it sized to the full size of the window, via MATCH_PARENT) https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:31: WebView wv = new WebView(this); I'd leave this being called webView so that you don't have to change the other references :) https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:56: wv.loadUrl("about:blank"); We probably want to also time how long it takes to 1) execute loadUrl here and 2) how long from that until we get an onPageFinished callback. The first load triggers startYourEngines() and is another important part of startup time for real apps, though I don't think it affects Android's EmptyWebViewActivity test.
hush@chromium.org changed reviewers: + hush@chromium.org
+sgurun https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:56: wv.loadUrl("about:blank"); On 2016/05/20 10:12:29, Torne wrote: > We probably want to also time how long it takes to 1) execute loadUrl here and > 2) how long from that until we get an onPageFinished callback. > > The first load triggers startYourEngines() and is another important part of > startup time for real apps, though I don't think it affects Android's > EmptyWebViewActivity test. Selim and I were talking about this. And his idea was that, since this metric is for WebView team internal consumption, we might as well measure it from the user's perspective. That is, from the start, to the point where a blank webview shown on the screen. We may be able to insert a visual state callback to achieve this.
The CQ bit was checked by yolandyan@google.com
The CQ bit was unchecked by yolandyan@google.com
https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:31: WebView wv = new WebView(this); On 2016/05/20 10:12:29, Torne wrote: > I'd leave this being called webView so that you don't have to change the other > references :) Done. https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:56: wv.loadUrl("about:blank"); On 2016/05/20 10:12:29, Torne wrote: > We probably want to also time how long it takes to 1) execute loadUrl here and > 2) how long from that until we get an onPageFinished callback. > > The first load triggers startYourEngines() and is another important part of > startup time for real apps, though I don't think it affects Android's > EmptyWebViewActivity test. I see, I would pass in more extra strings for each part we want to trace. Pls correct me if I understood it wrong, loadUrl is always asynchronised. Is there a point to do Tracing here in this Activity for loadUrl? Since the webview is just loading "about:blank" instead of a real web page, onPageFinished callback can be used to trace how startYourEngine load time? Or I can add trace event to startYourEngines() in WebViewChromium.java?
On 2016/05/20 22:45:15, yolandyan wrote: > https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... > File > android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java > (right): > > https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:31: > WebView wv = new WebView(this); > On 2016/05/20 10:12:29, Torne wrote: > > I'd leave this being called webView so that you don't have to change the other > > references :) > > Done. > > https://codereview.chromium.org/1994403002/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:56: > wv.loadUrl("about:blank"); > On 2016/05/20 10:12:29, Torne wrote: > > We probably want to also time how long it takes to 1) execute loadUrl here and > > 2) how long from that until we get an onPageFinished callback. > > > > The first load triggers startYourEngines() and is another important part of > > startup time for real apps, though I don't think it affects Android's > > EmptyWebViewActivity test. > > I see, I would pass in more extra strings for each part we want to trace. > > Pls correct me if I understood it wrong, loadUrl is always asynchronised. > Is there a point to do Tracing here in this Activity for loadUrl? > > Since the webview is just loading "about:blank" instead of a real web page, > onPageFinished callback can be used to trace how startYourEngine load time? Or I > can add trace event to startYourEngines() in WebViewChromium.java? Just timing how long it takes from "call loadUrl" to "onPageFinished" will give us the quickest possible "first page" time (i.e. how long does it take to load the first page when it's blank). That's what I was suggesting we time. You could add more detailed tracing inside startup as well, but if we *just* time startYourEngines() we may miss some other part of the process that's relevant to when the app thinks the page is loaded.
https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:60: Trace.endSection(); Do we need to have some kind of timeout here? If something goes wrong and onPageFinished is never called what will the test do? Is there already a timeout at a higher level that will take care of this acceptably?
https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:28: if (startUpTraceTag != null) { since you have similar pieces of code in multiple places, maybe having a helper traceFunc(String traceTag, boolean beginSection) or a pair of traceBeginSection(String traceTag)/traceEndSection(String traceTag) functions would simplify this a bit. https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:57: public void onPageFinished(WebView view, String url) { we should probably consider something else if we care about rendering, which looks like we don't in this instance anyway ;) https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:58: super.onPageFinished(view, url); doesn't really do anything, but may at some point in future? (not completely sure if this is our policy regarding override here, but sounds reasonable)
https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:28: if (startUpTraceTag != null) { On 2016/05/24 10:58:29, timvolodine wrote: > since you have similar pieces of code in multiple places, maybe having a helper > traceFunc(String traceTag, boolean beginSection) or a pair of > traceBeginSection(String traceTag)/traceEndSection(String traceTag) functions > would simplify this a bit. Done. https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:57: public void onPageFinished(WebView view, String url) { On 2016/05/24 10:58:29, timvolodine wrote: > we should probably consider something else if we care about rendering, which > looks like we don't in this instance anyway ;) Acknowledged. https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:58: super.onPageFinished(view, url); On 2016/05/24 10:58:29, timvolodine wrote: > doesn't really do anything, but may at some point in future? (not completely > sure if this is our policy regarding override here, but sounds reasonable) Acknowledged. https://codereview.chromium.org/1994403002/diff/20001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:60: Trace.endSection(); On 2016/05/23 12:49:00, Torne wrote: > Do we need to have some kind of timeout here? If something goes wrong and > onPageFinished is never called what will the test do? Is there already a timeout > at a higher level that will take care of this acceptably? Tracing uses package name + thread id + trace message to record the traces. If it times out, then the result is that this trace ends with the activity's thread (which can be a big regression). In chrome trace viewer, it shows these traces with "Did not finish" text Alex or I can look into failing the tests with no endSection call. Because the perf dashboard can't show whether a test is delayed because of timeouts or regression.
yolandyan@google.com changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:27: String startUpTraceTag = intent.getStringExtra("WebViewStartUpTraceTag"); After checking with Juan, consider the doing a trace doesn't cost much, and all the other tests that uses this activity aren't measuring time at this stage yet, we can simply always do tracing for this activity.
https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:27: String startUpTraceTag = intent.getStringExtra("WebViewStartUpTraceTag"); On 2016/05/26 21:21:17, yolandyan wrote: > After checking with Juan, consider the doing a trace doesn't cost much, and all > the other tests that uses this activity aren't measuring time at this stage yet, > we can simply always do tracing for this activity. Oh, huh. Yeah, it does seem like there's no need for this to be conditional at all if having a hardcoded tag is fine.
ping Not familiar with the codebase here. Does this look landable now? Or are there still improvements to make? Also, after this lands, how can I manually check the effect of this change? I assume launching the shell through an intent and then inspecting some logs?
lgtm with some comments below https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:27: String startUpTraceTag = intent.getStringExtra("WebViewStartUpTraceTag"); On 2016/05/27 09:52:39, Torne wrote: > On 2016/05/26 21:21:17, yolandyan wrote: > > After checking with Juan, consider the doing a trace doesn't cost much, and > all > > the other tests that uses this activity aren't measuring time at this stage > yet, > > we can simply always do tracing for this activity. > > Oh, huh. Yeah, it does seem like there's no need for this to be conditional at > all if having a hardcoded tag is fine. and also if there is no such tag, startUpTraceTag will be null and hence all the tracing a no-op. https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:27: String startUpTraceTag = intent.getStringExtra("WebViewStartUpTraceTag"); why not make this final as well? https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:31: setContentView(webView); just checking: this does not belong to startup right? https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:45: final String loadUrlTraceTag = intent.getStringExtra("WebViewLoadUrlTraceTag"); maybe move to one block with startUpTraceTag
https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... File android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java (right): https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:27: String startUpTraceTag = intent.getStringExtra("WebViewStartUpTraceTag"); On 2016/05/31 13:57:42, timvolodine wrote: > why not make this final as well? Done. https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:31: setContentView(webView); On 2016/05/31 13:57:42, timvolodine wrote: > just checking: this does not belong to startup right? Don't think so, I added an extra tracing to check, this part include all the inflate, layout work that android is doing. https://bugs.chromium.org/p/chromium/issues/detail?id=614372#c8 https://codereview.chromium.org/1994403002/diff/40001/android_webview/tools/s... android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/TelemetryActivity.java:45: final String loadUrlTraceTag = intent.getStringExtra("WebViewLoadUrlTraceTag"); On 2016/05/31 13:57:42, timvolodine wrote: > maybe move to one block with startUpTraceTag Done.
The CQ bit was checked by yolandyan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/1994403002/#ps60001 (title: "Hardcode default tracing tag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994403002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Mark trace data when launching WebView BUG=613419 ========== to ========== Mark trace data when launching WebView BUG=613419 Committed: https://crrev.com/a9fa54ee41fb8ef334883c808123268bea188003 Cr-Commit-Position: refs/heads/master@{#396905} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a9fa54ee41fb8ef334883c808123268bea188003 Cr-Commit-Position: refs/heads/master@{#396905}
Message was sent while issue was closed.
nednguyen@google.com changed reviewers: + nednguyen@google.com
Message was sent while issue was closed.
Why can we add these trace probes in the Webview's constructor? That's way, we can also have these data & metrics working for any android app that uses webview.
Message was sent while issue was closed.
On 2016/06/03 00:47:39, nednguyen wrote: > Why can we add these trace probes in the Webview's constructor? That's way, we > can also have these data & metrics working for any android app that uses > webview. Followed up about this on the bug. |
