New issue
Advanced search Search tips

Issue 748566 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 746251



Sign in to add a comment

Reuse shared state between story runs in Telemetry

Project Member Reported by perezju@chromium.org, Jul 25 2017

Issue description

We want to keep closing the browser between story runs by default, but be able to reuse the rest of the state (i.e. possible_browser, platform, network_controller).

Design doc:
https://docs.google.com/a/google.com/document/d/1fvvM276iJOhcGBnjYaOne4tcDtpplNLdR0_4S9o-Q-A/edit?usp=sharing

Splitting off from and blocking issue 746251
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b20c6301f009beeb0f79d47aa41d2f0e31578b9d

commit b20c6301f009beeb0f79d47aa41d2f0e31578b9d
Author: Juan A. Navarro Perez <perezju@chromium.org>
Date: Wed Jul 26 14:58:16 2017

[tools/perf] Remove BrowserStartupSharedState

There is no need to special case the shared state to close the browser
between story runs. This is now the default behaviour for all
benchmarks.

Bug:  748566 
Change-Id: Ic89ee1c9d8bab24c21237fc2b1d7cdc54d13565e
Reviewed-on: https://chromium-review.googlesource.com/586599
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489639}
[modify] https://crrev.com/b20c6301f009beeb0f79d47aa41d2f0e31578b9d/tools/perf/page_sets/startup_pages.py

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/012d60353af7a39b8507de30e9ec5dd4b65a0ae5

commit 012d60353af7a39b8507de30e9ec5dd4b65a0ae5
Author: Juan A. Navarro Perez <perezju@chromium.org>
Date: Tue Aug 08 09:59:06 2017

[tools] Remove "restart_after_each_page" from LegacyPageTest clients

This option of the LegacyPageTest constructor is being removed. It is
no longer needed since the current default behaviour already is to
close the browser in between story runs.

This change is expected to be a no-op and not affect any measurements.

Bug:  748566 
Change-Id: I8c0788208544c0a7898c01d744edc0867fdf369b
Reviewed-on: https://chromium-review.googlesource.com/589010
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492581}
[modify] https://crrev.com/012d60353af7a39b8507de30e9ec5dd4b65a0ae5/tools/chrome_proxy/common/chrome_proxy_measurements.py
[modify] https://crrev.com/012d60353af7a39b8507de30e9ec5dd4b65a0ae5/tools/chrome_proxy/integration_tests/chrome_proxy_measurements.py
[modify] https://crrev.com/012d60353af7a39b8507de30e9ec5dd4b65a0ae5/tools/perf/measurements/smoothness.py
[modify] https://crrev.com/012d60353af7a39b8507de30e9ec5dd4b65a0ae5/tools/perf/measurements/startup.py

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1111d0e32444b5c4aed72840baf8fcc62d6a599

commit e1111d0e32444b5c4aed72840baf8fcc62d6a599
Author: Juan A. Navarro Perez <perezju@chromium.org>
Date: Tue Aug 08 14:49:20 2017

[tools] Remove client overrides of StopBrowserAfterPage

It's now the default behavior of all benchmarks to close the browser
between story runs. So there is no need to specify it here.

This change should be a no-op.

Bug:  748566 
Change-Id: I525e2f141f419c9561867e770138a0c30381e3b4
Reviewed-on: https://chromium-review.googlesource.com/584775
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492619}
[modify] https://crrev.com/e1111d0e32444b5c4aed72840baf8fcc62d6a599/tools/chrome_proxy/common/chrome_proxy_measurements.py
[modify] https://crrev.com/e1111d0e32444b5c4aed72840baf8fcc62d6a599/tools/perf/measurements/image_decoding.py

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7433c1a259abfa6839c22090867c11e0641de134

commit 7433c1a259abfa6839c22090867c11e0641de134
Author: catapult-deps-roller@chromium.org <catapult-deps-roller@chromium.org>
Date: Wed Aug 09 11:48:48 2017

Roll src/third_party/catapult/ 26fa809f1..9d0d9fc45 (1 commit)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/26fa809f1faa..9d0d9fc4552b

$ git log 26fa809f1..9d0d9fc45 --date=short --no-merges --format='%ad %ae %s'
2017-08-09 perezju [Telemetry] Remove support for StopBrowserAfterPage on LegacyPageTest

Created with:
  roll-dep src/third_party/catapult
BUG= 748566 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: Ica21a4ced3e5281e2cb4c5bca34360c39534bd50
Reviewed-on: https://chromium-review.googlesource.com/607414
Reviewed-by: <catapult-deps-roller@chromium.org>
Commit-Queue: <catapult-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492943}
[modify] https://crrev.com/7433c1a259abfa6839c22090867c11e0641de134/DEPS

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8e804f84846d0ebaff468455b46b62ff1932a24

commit f8e804f84846d0ebaff468455b46b62ff1932a24
Author: catapult-deps-roller@chromium.org <catapult-deps-roller@chromium.org>
Date: Wed Aug 09 16:03:03 2017

Roll src/third_party/catapult/ 1a1b45a57..868342d85 (2 commits)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/1a1b45a57256..868342d8543e

$ git log 1a1b45a57..868342d85 --date=short --no-merges --format='%ad %ae %s'
2017-08-09 perezju [Telemetry] Remove needs_browser_restart_after_each_page option
2017-08-09 loloangela Remove interface-not-implemented from pylintrc file

Created with:
  roll-dep src/third_party/catapult
BUG= 748566 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: I18ddaab2ff306972f5a1755354a95a7403b5cf90
Reviewed-on: https://chromium-review.googlesource.com/608548
Reviewed-by: <catapult-deps-roller@chromium.org>
Commit-Queue: <catapult-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493005}
[modify] https://crrev.com/f8e804f84846d0ebaff468455b46b62ff1932a24/DEPS

Most remaining changes are reviewed and ready to land, just documenting here the order in which they should land, in case anything goes wrong and something needs to be reverted.

1. [catapult] Small API change in ShouldCloseBrowserAfterStoryRun
-- Landing now.
   https://codereview.chromium.org/2994833002/

2. [chromium] Remove test about ShouldTearDownStare
-- Landing now.
   https://chromium-review.googlesource.com/c/610160

3. [chromium] Migrate benchmarks that need to override default
   browser-closing behavior.
-- Will land tomorrow (Friday), depends on 1.
   https://chromium-review.googlesource.com/c/610044/

4. [catapult] Reuse shared state between story runs (finally!)
-- Will land next week Monday if all looks ok, must land after
   3, or those benchmarks will break.
   https://codereview.chromium.org/2998773002/

After that only a few cleanup CLs may be needed and we should be
done. Whew!
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0c77c7942d0f2270110d92f619a117105d5fef2

commit c0c77c7942d0f2270110d92f619a117105d5fef2
Author: Juan A. Navarro Perez <perezju@chromium.org>
Date: Thu Aug 10 16:15:16 2017

[system health] Remove ShouldTearDownState test

The ShouldTearDownStateAfterEachStoryRun method is going to be removed
by https://codereview.chromium.org/2998773002

This test is no longer needed.

Bug:  748566 
Change-Id: Iba8d4e2bef37ea96347ec802660d4c938e89f8d9
Reviewed-on: https://chromium-review.googlesource.com/610160
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493411}
[modify] https://crrev.com/c0c77c7942d0f2270110d92f619a117105d5fef2/tools/perf/benchmarks/system_health_unittest.py

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2de71eade34e55b034df7b016e3e66fc9673c303

commit 2de71eade34e55b034df7b016e3e66fc9673c303
Author: catapult-deps-roller@chromium.org <catapult-deps-roller@chromium.org>
Date: Fri Aug 11 16:54:37 2017

Roll src/third_party/catapult/ 0eeb5baed..7e08027e9 (15 commits)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/0eeb5baed7a4..7e08027e9800

$ git log 0eeb5baed..7e08027e9 --date=short --no-merges --format='%ad %ae %s'
2017-08-11 perezju [experimental] Chrome on Android Go user stories
2017-08-11 jbudorick Revert of ApkHelper: Add GetViewActivityName(). Make GetActivityName() more strict (patchset #2 id:20001 of https://codereview.chromium.org/2994053002/ )
2017-08-11 perezju Revert of [devil] Extract logging common behavior to its own module. (patchset #3 id:40001 of https://codereview.chromium.org/2972253002/ )
2017-08-10 jbudorick [devil] Disable window animations in device provisioning.
2017-08-10 htwiggsmith Update tag filter to return all stories when no tags are selected
2017-08-10 dskiba Write startup HTML profile into the specified file.
2017-08-10 phsilva Gathering Device Information in Telemetry
2017-08-10 eakuefner [Catapult] Remove special-purpose diagnostic addition modules
2017-08-10 htwiggsmith Add some Google Analytics events to the dashboard
2017-08-10 benjhayden Rename diagnostic reserved names to camelCase.
2017-08-10 loloangela Fix errors related to invalid-name pt. 2
2017-08-10 loloangela Fix errors related to invalid-name pt. 1
2017-08-10 perezju [Telemetry] Change API of ShouldStopBrowserAfterStoryRun
2017-08-10 agrieve ApkHelper: Add GetViewActivityName(). Make GetActivityName() more strict
2017-08-10 jbudorick [devil] Extract logging common behavior to its own module.

Created with:
  roll-dep src/third_party/catapult
BUG=752963, 749239 ,753077, 748566 , 749239 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: Ie9e36907f7b9cfbef956a2c01eec99bd8d262e8f
Reviewed-on: https://chromium-review.googlesource.com/612370
Reviewed-by: <catapult-deps-roller@chromium.org>
Commit-Queue: <catapult-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493775}
[modify] https://crrev.com/2de71eade34e55b034df7b016e3e66fc9673c303/DEPS

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6879ad2b08f6d9f23f144f12305eaad143eccc3c

commit 6879ad2b08f6d9f23f144f12305eaad143eccc3c
Author: Juan A. Navarro Perez <perezju@chromium.org>
Date: Sat Aug 12 07:29:08 2017

[tools/perf] Switch benchmarks that want to keep browser open

As ShouldTearDownState* methods are going away, benchmarks that
need such functionality to control whether and when the browser
should be closed in between story runs must override the shared
state and code the desired behavior.

This CL migrates all such benchmarks:

- memory.top_10_mobile
- memory.long_running_desktop_sites
- memory.dual_browser_test
- memory.long_running_dual_browser_test

Changes will be in effect as the CL lands. No changes in
performance metrics are expected.

Depends on Telemetry API change:
https://codereview.chromium.org/2994833002/

Bug:  748566 
Change-Id: Ibdcabd4bafa05250e2db14e5a446f13a64ebe67a
Reviewed-on: https://chromium-review.googlesource.com/610044
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#493968}
[modify] https://crrev.com/6879ad2b08f6d9f23f144f12305eaad143eccc3c/tools/perf/benchmarks/memory.py
[modify] https://crrev.com/6879ad2b08f6d9f23f144f12305eaad143eccc3c/tools/perf/contrib/memory_extras/memory_extras.py
[modify] https://crrev.com/6879ad2b08f6d9f23f144f12305eaad143eccc3c/tools/perf/page_sets/desktop_memory.py
[modify] https://crrev.com/6879ad2b08f6d9f23f144f12305eaad143eccc3c/tools/perf/page_sets/dual_browser_story.py
[modify] https://crrev.com/6879ad2b08f6d9f23f144f12305eaad143eccc3c/tools/perf/page_sets/memory_top_10_mobile.py

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f450813f36fabf74d1a9cdee2f80b56dfc9814fc

commit f450813f36fabf74d1a9cdee2f80b56dfc9814fc
Author: catapult-deps-roller@chromium.org <catapult-deps-roller@chromium.org>
Date: Mon Aug 14 14:34:17 2017

Roll src/third_party/catapult/ 5db51352a..122dd5e91 (1 commit)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/5db51352aa5c..122dd5e91bf6

$ git log 5db51352a..122dd5e91 --date=short --no-merges --format='%ad %ae %s'
2017-08-14 perezju [Telemetry] Reuse shared state between story runs

Created with:
  roll-dep src/third_party/catapult
BUG= 748566 ,746251


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: I2df8dd2b36a33be08c5ac3cce0c749df7b8e91d0
Reviewed-on: https://chromium-review.googlesource.com/613181
Reviewed-by: <catapult-deps-roller@chromium.org>
Commit-Queue: <catapult-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494047}
[modify] https://crrev.com/f450813f36fabf74d1a9cdee2f80b56dfc9814fc/DEPS

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4349e89d4aa96d5f1847d8ddb85707244a49abc

commit a4349e89d4aa96d5f1847d8ddb85707244a49abc
Author: Juan A. Navarro Perez <perezju@chromium.org>
Date: Tue Aug 15 11:22:20 2017

[tools/perf] Remove ShouldTearDownState* methods

These are no longer used.

Bug:  748566 
Change-Id: I7296124e69dbf909b181b23d71109948ead50bf6
Reviewed-on: https://chromium-review.googlesource.com/615165
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494359}
[modify] https://crrev.com/a4349e89d4aa96d5f1847d8ddb85707244a49abc/tools/perf/benchmarks/memory.py
[modify] https://crrev.com/a4349e89d4aa96d5f1847d8ddb85707244a49abc/tools/perf/contrib/memory_extras/memory_extras.py

Status: Fixed (was: Started)
This is done!

Sign in to add a comment