Allow multiple browser restarts with TMBv2 stories on Android |
||||||
Issue description
For Android startup benchmarks we need a way to Close() the browser, and start one while the story is running.
The desired code (in terms of examples/benchmarks/android_go_benchmark.py) would look something like:
class _AndroidStartupWithIntentStory(story_module.Story):
def __init__(self):
...
def Run(self, state):
trace_values = []
for i in xrange(10):
state.LaunchBrowser('http://bbc.co.uk')
browser = state.FindBrowser()
browser.foreground_tab.action_runner.tab.WaitForDocumentReadyStateToBeComplete()
trace_values.append(browser.Close())
# The following trace joining is not under discussion right now.
self._MagicallySaveTracesForLaterMerging(trace_values)
In the code above if LaunchBrowser() only starts an intent (as in android_go_benchmark.py), then on the iteration i==1 the browser.Close() has deleted command line flags, ran setenforce 1, etc.
The LaunchBrowser() should *not* Create the possible browser, as this opens Chrome with about:blank, instead we want just to SetUpEnvironment() for the intent to launch Chrome, devtools to connect correctly, etc.
I see a few ways to do it, the simplest would be to allow multiple calls to SetUpEnvironment() like this:
diff --git a/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py b/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py
index 2e70eaef0..be538bca2 100644
--- a/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py
+++ b/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py
@@ -135,7 +135,8 @@ class PossibleAndroidBrowser(possible_browser.PossibleBrowser):
self._backend_settings.profile_ignore_list)
def SetUpEnvironment(self, browser_options):
- super(PossibleAndroidBrowser, self).SetUpEnvironment(browser_options)
+ if not self._browser_options:
+ super(PossibleAndroidBrowser, self).SetUpEnvironment(browser_options)
self._platform_backend.DismissCrashDialogIfNeeded()
device = self._platform_backend.device
startup_args = self.GetBrowserStartupArgs(self._browser_options)
... then state.LaunchBrowser() would do self._possible_browser.SetUpEnvironment(self._finder_options.browser_options)
This also eliminates the need to run SetUpEnvironment() in SharedAndroidStoryState.WillRunStory(), which makes the control flow between SetUp and browser.Close() more straightforward.
This hack, however, looks not ideal, as different classes are responsible for creating and tearing down the browser environment. Perhaps this functionality can be extracted into a separate utility?
Additionally, since OS pagecache clearing is configured in browser_options (clear_sytem_cache_for_browser_and_profile_on_start), in the future we may need to change the options during a story run, though I cannot say it is required right now.
Another possibility is to introduce a mode into the class Browser that would avoid tearing down the environment (does not have to be re-created). This will make benchmarking slightly faster, but I am not sure how difficult it would be to re-establish a devtools connection. There could be other problems arising from Browser lifetime spanning several browser restarts on the device - not sure it would be a good abstraction then.
,
Jun 6 2018
sure np! with the workaround in android_browser_finder I am not blocked on it right now
,
Jun 6 2018
,
Jun 11 2018
Similar to the problem of setting up the environment via PossibleBrowser, we have TracingControllerBackend.StartTracing() initialized from story_runner (test.WillRunStory). When we call browser.Close(), the TracingController._current_state is reset in StopTracing(), even though the devtools connection should be accessible. Again, creating state in one place, tearing down in another. I'm looking for a temporary workaround.
,
Jun 12 2018
Started looking into into the current tangle that we have. When ready I'll share a design doc to discuss possible solutions.
,
Jun 13 2018
I had a look at this. My thoughts are that we should be calling SetUpEnvironment() only *once* at the start of the story and CleanUpEnvironment() *once* when the story finishes. Note that browser.Close() by itself does *not* clean up the environment (so it doesn't remove command line flags, run setenforce 1, etc.) In principle (I think, but haven't checked) it should be possible to open/close the browser several times in between the SetUp-/CleanUpEnvironment pair of calls. If this is not working it's a bug, as the current architecture/implementation is meant to allow this. I think the only tricky bit is that the trace must be collected from the browser before closing it; so what we may be missing is adding functionality for the TracingController to incrementally collect traces from different "browsers" at different times. I have the feeling that this should be doable with some little work (something like this already happens in memory.dual_browser_test; where we collect traces from two "browsers"). Just need to figure out the right way to plumb this. In case it helps I wrote this to figure out how the different Telemetry layers interact in the current architecture: https://docs.google.com/document/d/1l4CIqZACAlyogsbPESz0IY3FS4bS7f-DimVQC1uM-os/edit?usp=sharing
,
Jun 13 2018
That's similar to what I discuss with pasko@ last time. My thinking is when we call browser.Close(..), chrome_browser_backend will check if tracing is enabled, and if so, it will invoke chrome_tracing_agent to collect the trace & save it to some temp buffer before actually closing the browser
,
Jun 14 2018
> I had a look at this. My thoughts are that we should be calling > SetUpEnvironment() only *once* at the start of the story and > CleanUpEnvironment() *once* when the story finishes. That'd be good. > Note that browser.Close() by itself does *not* clean up the environment (so it > doesn't remove command line flags, run setenforce 1, etc.) > > In principle (I think, but haven't checked) it should be possible to > open/close the browser several times in between the SetUp-/CleanUpEnvironment > pair of calls. If this is not working it's a bug, as the current > architecture/implementation is meant to allow this. It does not work out of the box because FindBrowser clears out command line flags: https://codesearch.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py?q=_GetBrowserInstance&sq=package:chromium&g=0&l=206 The trace config, on the other hand, is removed by browser.Close(): https://codesearch.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py?type=cs&q=StopAgentTracing+file:chrome_tracing_agent.py&sq=package:chromium&g=0&l=204 > I think the only tricky bit is that the trace must be collected from the > browser before closing it; so what we may be missing is adding functionality > for the TracingController to incrementally collect traces from different > "browsers" at different times. I am leaning towards thinner support in tracing controller and browser because they already manage a bit of state. Increasing the amount of guessing what layers above are doing seems more complicated than making browser.Close() return a trace (trace_data). With such, the story saves trace_data's on a side and then puts them back into |results| for timeline_based_measurement.Measure(). That's what I am doing in my prototype while figuring out trace merging. I realize this might be too big of departure from Telemetry design, so I cannot insist. From my experience building tools/android/loading (which probably does not work any more) I found it easier to drive all the interactions with device/chrome from the test/benchmark itself without pushing state down to classes dealing with the device. It was easy to have a _library_ of various contextmanagers that we tossed around in various ways to serve quite different testing scenarios (loading, CCT, saving/restoring disk cache, etc.). > I have the feeling that this should be doable with some little work (something > like this already happens in memory.dual_browser_test; where we collect traces > from two "browsers"). Just need to figure out the right way to plumb this. > > In case it helps I wrote this to figure out how the different Telemetry layers > interact in the current architecture: > https://docs.google.com/document/d/1l4CIqZACAlyogsbPESz0IY3FS4bS7f-DimVQC1uM-os/edit?usp=sharing It helps, thank you! I spent a bit of time to discover it last week :) From Ned: > That's similar to what I discuss with pasko@ last time. My thinking is when we > call browser.Close(..), chrome_browser_backend will check if tracing is > enabled, and if so, it will invoke chrome_tracing_agent to collect the trace & > save it to some temp buffer before actually closing the browser I am not sure what 'tracing is enabled' means in Telemetry and why we need it :) For startup benchmarks tracing must be 'enabled' in Chrome for all of browser process lifetime, and it would not hurt even to have it enabled when the browser is not running :) It the trace config that tells to enable tracing at start, among other things. In theory having everything piped through trace config should simplify Telemetry code: one does not need to bother with starting/stopping tracing at appropriate times, only setting up the 'environment' is needed. We get the trace when we call StopTracing(), or maybe we should not even stop it, it should DumpTrace() and continue with tracing. In practice, however, we are starting/stopping, and trying to avoid starting for startup tracing in clever ways, as a special case. Though I am not sure how much it would involve to actually simplify it this way for all benchmarks. WDYT?
,
Jun 14 2018
> It does not work out of the box because FindBrowser clears out command line > flags: > https://codesearch.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py?q=_GetBrowserInstance&sq=package:chromium&g=0&l=206 Ah, yes. That's a bug. We should be able to just remove that line. The file will be later removed during CleanupEnvironment (as originally intended). > The trace config, on the other hand, is removed by browser.Close(): I'm not finding where browser.Close() ends up calling StopAgentTracing?
,
Jun 14 2018
> > The trace config, on the other hand, is removed by browser.Close(): > > I'm not finding where browser.Close() ends up calling StopAgentTracing? sorry, it is in my prototype: I am calling self._platform_backend.platform.tracing_controller.StopTracing() from browser.Close() in order to grab a trace. Wherever StopTracing is called, it would remove the trace config. Sounds like we should move the trace config deletion somewhere, perhaps CleanupEnvironment() would be appropriate?
,
Jun 14 2018
Got it. I think that's the issue that will go away with Ned's CL: https://chromium-review.googlesource.com/c/catapult/+/1100745?
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/catapult/+/d02fd6682709703186b5b8ace092ddc41f5cbd8e commit d02fd6682709703186b5b8ace092ddc41f5cbd8e Author: Juan Antonio Navarro Perez <perezju@chromium.org> Date: Fri Jun 15 11:11:42 2018 [Telemetry] Do not clear Chrome command line on Android after create The file might be needed there if the story decides e.g. to close and re-open the browser. When the story finishes, the flags will be reset by CleanupEnvironment. Bug: chromium:849703 Change-Id: I2701e4874043edad956adfada34314d7f7d75b65 Reviewed-on: https://chromium-review.googlesource.com/1100887 Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> [modify] https://crrev.com/d02fd6682709703186b5b8ace092ddc41f5cbd8e/telemetry/telemetry/internal/browser/browser_unittest.py [modify] https://crrev.com/d02fd6682709703186b5b8ace092ddc41f5cbd8e/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b88bb6843c87255a0c2a6c0259a9028e12be238b commit b88bb6843c87255a0c2a6c0259a9028e12be238b Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Fri Jun 15 12:10:28 2018 Roll src/third_party/catapult 67c2514bf571..d02fd6682709 (1 commits) https://chromium.googlesource.com/catapult.git/+log/67c2514bf571..d02fd6682709 git log 67c2514bf571..d02fd6682709 --date=short --no-merges --format='%ad %ae %s' 2018-06-15 perezju@chromium.org [Telemetry] Do not clear Chrome command line on Android after create Created with: gclient setdep -r src/third_party/catapult@d02fd6682709 The AutoRoll server is located here: https://catapult-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel BUG= chromium:849703 TBR=sullivan@chromium.org Change-Id: I6c647e93a0655db67aed5c093f5b0eb0bcc23b70 Reviewed-on: https://chromium-review.googlesource.com/1102397 Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#567610} [modify] https://crrev.com/b88bb6843c87255a0c2a6c0259a9028e12be238b/DEPS
,
Jun 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/catapult/+/4563784b9387d1432517565bab7bac19969bb418 commit 4563784b9387d1432517565bab7bac19969bb418 Author: Juan Antonio Navarro Perez <perezju@chromium.org> Date: Thu Jun 21 13:05:32 2018 [Telemetry] Example: restart browser during story A few bugs prevent us from restarting the browser for the duration of a story ( crbug.com/854212 ), this CL includes an example of workarounds that a benchmark can use to avoid those bugs. Also includes a small change in tab.Close() to allow closing the last tab of the browser. This was previously prevented by the tab backend. By default the old behavior is retained, but an option to override this is now provided. Bug: chromium:849703 Change-Id: I44000226a69100366c1cc04a1378badd219421f6 Reviewed-on: https://chromium-review.googlesource.com/1104168 Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> [modify] https://crrev.com/4563784b9387d1432517565bab7bac19969bb418/telemetry/examples/.gitignore [modify] https://crrev.com/4563784b9387d1432517565bab7bac19969bb418/telemetry/telemetry/internal/backends/chrome/tab_list_backend.py [modify] https://crrev.com/4563784b9387d1432517565bab7bac19969bb418/telemetry/telemetry/internal/browser/tab.py [modify] https://crrev.com/4563784b9387d1432517565bab7bac19969bb418/telemetry/examples/benchmarks/android_go_benchmark.py
,
Jun 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d370bf6746dba7d05a325c314cfc12b2693beb9c commit d370bf6746dba7d05a325c314cfc12b2693beb9c Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Thu Jun 21 18:47:17 2018 Roll src/third_party/catapult 44fc6f687b35..53842b6cbc1f (11 commits) https://chromium.googlesource.com/catapult.git/+log/44fc6f687b35..53842b6cbc1f git log 44fc6f687b35..53842b6cbc1f --date=short --no-merges --format='%ad %ae %s' 2018-06-21 ckitagawa@chromium.org [Devil] Fix pylint typecheck errors 2018-06-21 perezju@chromium.org [Telemetry] Add clear_caches option to possible_browser.Create 2018-06-21 charliea@chromium.org Tell sqlite3 to use a write-ahead mode 2018-06-21 perezju@chromium.org Fix index of timeseries data frames 2018-06-21 perezju@chromium.org [Telemetry] Do not cache installed apps in AndroidPlatformBackend 2018-06-21 benjhayden@chromium.org Increase Anomaly query limit in graph_json. 2018-06-21 nednguyen@google.com Disable testWaitForJavaScriptCondition on Mac 2018-06-21 perezju@chromium.org [Telemetry] Example: restart browser during story 2018-06-21 perezju@chromium.org [Telemetry] Fix pylint unidiomatic-typecheck warnings 2018-06-21 perezju@chromium.org [soundwave] Code health in dashboard_api.py 2018-06-21 perezju@chromium.org [soundwave] Follow cursor on fetch alerts API call Created with: gclient setdep -r src/third_party/catapult@53842b6cbc1f The AutoRoll server is located here: https://catapult-roll.skia.org Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel BUG=chromium:811244,chromium:711073,chromium:854744, chromium:849703 , chromium:854986 TBR=sullivan@chromium.org Change-Id: I7a6383560447cee344a85996f9a7f595e0e9088c Reviewed-on: https://chromium-review.googlesource.com/1110297 Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#569338} [modify] https://crrev.com/d370bf6746dba7d05a325c314cfc12b2693beb9c/DEPS
,
Jun 22 2018
I think this is "fixed", at least no longer blocking issue 760498. On issue 854212 I'll track the follow up work to remove the workarounds.
,
Jan 16
(6 days ago)
,
Jan 16
(6 days ago)
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by perezju@chromium.org
, Jun 6 2018Status: Assigned (was: Available)