Investigate Safe Browsing startup |
||
Issue description
AwSafeBrowsingConfigHelper.maybeEnableSafeBrowsingFromManifest takes a bit of time upstream (0.7ms on the bot), but seems to take a bit of time downstream (36ms in attached trace).
Part of this is due to runtime class verification of GoogleApiAvailability (downstream), but there may be other upstream factors.
Initial investigation (in the attached trace, which has extra TraceEvents) shows this may be due to the JNI plumbing for {get,set}safebrowsingenabledbymanifest.
We can probably remove the native side, completely removing the need for JNI.et
,
Jul 2
A single JNI call shouldn't ever be taking even 1ms of overhead.
,
Jul 2
It seems to be (JNI + invoke another native function + obtain AutoLock + set global variable). That's all nativeSetSafeBrowsingEnabledByManifest does. I'm looking here [1] and here [2] [1] https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents_statics.cc?l=112&rcl=7667e30fdbee03129ff475e122a7fd28afa4f973 [2] https://cs.chromium.org/chromium/src/android_webview/browser/aw_safe_browsing_config_helper.cc?l=22&rcl=a8b6fd78bbb8b1022797e98674cdd7c87a66c530
,
Jul 2
Note: this may not be a "downstream" vs. "upstream" issue, but an issue of "local run" vs. "on the bot".
,
Jul 3
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/14e73ba4a40000
,
Jul 3
*All numbers are taken locally on my device* Currently, maybeEnableSafeBrowsingFromManifest takes an average of 32 ms (locally). * The latter portion of this event (8 ms on average), is canUseGmsInternal. This method is slow because of class verification issues. * The beginning portion (24 ms on average) is spent invoking AwContentsStatics.setSafeBrowsingEnabledByManifest * If I remove the native AwSBConfigHelper and the AwContentsStatics methods, maybeEnableSafeBrowsingFromManifest only takes an average of 9 ms. canUseGmsInternal has similar class verification penalties, so that's 1-2 ms to set the static variable. * No conclusion for WebViewStartupInterval. Durations vary quite a bit between local runs (std dev is 350 ms, for an average duration of 1726 ms). Pinpoint may be able to give a clearer answer (uploaded in http://crrev/c/1123887). --- Even if http://crrev/c/1123887 doesn't fix startup issues, I think the CL is beneficial because it simplifies some unnecessary complexity.
,
Jul 3
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/14e73ba4a40000
,
Jul 3
No significant difference in the CL when running the pinpoint. The CL is still a benefit, because it reduces code complexity. But perhaps we can identify a different cause for concern. So, it may be beneficial to add more trace events (http://crrev/c/1123401).
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0 commit cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0 Author: Nate Fischer <ntfschr@chromium.org> Date: Tue Jul 03 21:52:44 2018 AW: remove native side of AwSafeBrowsingConfigHelper This removes the native side of AwSafeBrowsingConfigHelper and the related AwContentsStatics methods. The native AwSafeBrowsingConfigHelper previously only maintained a global variable to keep track of manifest opt-in. However, we currently only set/get that value from the Java side. Therefore, we can remove the native side and keep track of this value in a static Java variable. This appears to be the culprit for some startup delay, so this is expected to improve startup time as well. R=torne@chromium.org conditions (including manifest opt-in/out) (seems to be a noticeable startup win, net reduction of 23 ms locally) Bug: 859696 Test: manual - use SuperSafeBrowsing test app, check a variety of opt-in Test: tools/perf/run_benchmark system_health.webview_startup --browser android-webview-google Change-Id: I520c405027991101b5c8c23c58853c6b7c73c2b3 Reviewed-on: https://chromium-review.googlesource.com/1123887 Commit-Queue: Nate Fischer <ntfschr@chromium.org> Reviewed-by: Richard Coles <torne@chromium.org> Cr-Commit-Position: refs/heads/master@{#572366} [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/BUILD.gn [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/browser/aw_contents_statics.cc [delete] https://crrev.com/0e3be792c60251e6d90a6995b9f66460f61ea0ce/android_webview/browser/aw_safe_browsing_config_helper.cc [delete] https://crrev.com/0e3be792c60251e6d90a6995b9f66460f61ea0ce/android_webview/browser/aw_safe_browsing_config_helper.h [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/java/src/org/chromium/android_webview/AwContentsStatics.java [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/java/src/org/chromium/android_webview/AwServiceWorkerController.java [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/java/src/org/chromium/android_webview/AwSettings.java [modify] https://crrev.com/cfd87c24aea4731c9b88a9c083d0a17d1ad3b3c0/android_webview/lib/aw_main_delegate.cc
,
Jul 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/307666b310459327d2e3839bf89d308fdbf65175 commit 307666b310459327d2e3839bf89d308fdbf65175 Author: Nate Fischer <ntfschr@chromium.org> Date: Tue Jul 10 22:08:22 2018 AW: trace events for safe browsing startup No change to logic. This adds trace events to better capture events during WebView's Safe Browsing startup. R=torne@chromium.org Bug: 859696 Test: N/A Change-Id: I13d59093c2bf80bc11f59197732c97a2be9a1cbe Reviewed-on: https://chromium-review.googlesource.com/1123401 Reviewed-by: Richard Coles <torne@chromium.org> Commit-Queue: Nate Fischer <ntfschr@chromium.org> Cr-Commit-Position: refs/heads/master@{#573942} [modify] https://crrev.com/307666b310459327d2e3839bf89d308fdbf65175/android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java
,
Jul 16
Changwan, is the perf bot (or dashboard) broken [1]? I haven't seen any runs since July 3rd [2]. Let me know if I'm reading this wrong or have the wrong links. I think the bot was broken when I landed comment #10, so maybe that's the cause? [1] https://chromeperf.appspot.com/report?sid=5ae87c41efb3d436419396e0551577a6e5bb1dc9544c55bf0e2881db28ed54be [2] https://chromium.googlesource.com/chromium/src/+log/ef37136f805f4172ff68727982ef36b129726508%5E..cca55c33d0728f8429a6ad8ed92a9625171970be?pretty=fuller&n=1000
,
Aug 17
We can save 0.3ms if we fetch manifest opt-in with a posted task.
,
Aug 17
,
Aug 17
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/11a1d234640000
,
Aug 17
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/11a1d234640000
,
Aug 17
I ran a pinpoint on a proposed CL (https://chromium-review.googlesource.com/c/chromium/src/+/1180252). The pinpoint showed a slight regression. I don't think there's further work (any potential gains would be very minimal anyway), so I'll close this. |
||
►
Sign in to add a comment |
||
Comment 1 by ntfschr@chromium.org
, Jul 2