New issue
Advanced search Search tips

Issue 792860 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 784319



Sign in to add a comment

Kill platform_backend.DidCreateBrowser, platform_backend. DidStartBrowser, platform_backend. WillCloseBrowser hooks in Telemetry

Project Member Reported by nedngu...@google.com, Dec 7 2017

Issue description

These hooks is problematic for a few reason:
1) They makes assumption that the logic of creating a browser is explicitly controlled by Telemetry framework, but it's not necessarily True for case like a user story clicking on screen & that happens to launch a browser.
2) It currently try to SetFullPerformanceModeEnabled & unset based on the number of browesrs object that Telemetry is able to keep track of, which make reasoning about life cycle of Telemetry perf test harder. T
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0

commit 6c03f202337a29da9ac37e88c7e0d3e65a72f9d0
Author: Nghia Nguyen <nednguyen@google.com>
Date: Thu Dec 07 15:40:30 2017

Remove platform_backend's DidStartBrowser, DidCreateBrowser and WillCloseBrowser hooks

The only part of these hooks that create actual effect on perf test is
the calls to SetFullPerformanceModeEnabled in DidCreateBrowser & unset it in
WillCloseBrowser. As we remove those hooks, those calls are moved to
shared_page_state for now (ideally they should be invoked in story_runner so
other shared_state can reuse this, but
the code there is already way to complexed, so I just leave it there for now).

The life cycle of SetFullPerformanceModeEnabled now is:

SetFullPerformanceModeEnabled(...) is called before any test is run (before
browser is started) in SharedPageState.__init__

SetFullPerformanceModeEnabled(False) is called after all the test has run
in SharedPageState.TearDownState().

Compared with before, we run SetFullPerformanceModeEnabled less often & this is
a good optimization as there is no need to set performance mode many times.

Notes that this also changes relative order of the ClearCaches &
SetFullPerformanceModeEnabled as following:

Before:
ClearCaches() (in desktop_browser_finder.Create)
SetFullPerformanceModeEnabled(...) (in browser.__init__)

After:

SetFullPerformanceModeEnabled(...) (in shared_state.__init__)
ClearCaches() (in desktop_browser_finder.Create, in shared_state.WillRunStory)

Hence with this CL, the relative order of these two events would be back to what they were before
the regression in crbug/792357

Bug:  chromium:792860 
Bug: chromium:792357
Change-Id: Ibeba9ca0af55921514d4f3fecf692c3a740b1aac
Reviewed-on: https://chromium-review.googlesource.com/814214
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0/telemetry/telemetry/core/platform.py
[modify] https://crrev.com/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0/telemetry/telemetry/internal/platform/platform_backend.py
[modify] https://crrev.com/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0/telemetry/telemetry/page/shared_page_state.py
[modify] https://crrev.com/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0/telemetry/telemetry/internal/browser/browser.py
[modify] https://crrev.com/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0/telemetry/telemetry/testing/fakes/__init__.py
[modify] https://crrev.com/6c03f202337a29da9ac37e88c7e0d3e65a72f9d0/telemetry/telemetry/internal/browser/browser_unittest.py

Project Member

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

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475

commit 3572e89d92c2ce8d58ab62ad7fde5e2f628c1475
Author: Ned Nguyen <nednguyen@google.com>
Date: Fri Dec 08 11:55:45 2017

Revert "Remove platform_backend's DidStartBrowser, DidCreateBrowser and WillCloseBrowser hooks"

This reverts commit 6c03f202337a29da9ac37e88c7e0d3e65a72f9d0.

Reason for revert: this may break webgl_conformance_integration_test

Original change's description:
> Remove platform_backend's DidStartBrowser, DidCreateBrowser and WillCloseBrowser hooks
> 
> The only part of these hooks that create actual effect on perf test is
> the calls to SetFullPerformanceModeEnabled in DidCreateBrowser & unset it in
> WillCloseBrowser. As we remove those hooks, those calls are moved to
> shared_page_state for now (ideally they should be invoked in story_runner so
> other shared_state can reuse this, but
> the code there is already way to complexed, so I just leave it there for now).
> 
> The life cycle of SetFullPerformanceModeEnabled now is:
> 
> SetFullPerformanceModeEnabled(...) is called before any test is run (before
> browser is started) in SharedPageState.__init__
> 
> SetFullPerformanceModeEnabled(False) is called after all the test has run
> in SharedPageState.TearDownState().
> 
> Compared with before, we run SetFullPerformanceModeEnabled less often & this is
> a good optimization as there is no need to set performance mode many times.
> 
> Notes that this also changes relative order of the ClearCaches &
> SetFullPerformanceModeEnabled as following:
> 
> Before:
> ClearCaches() (in desktop_browser_finder.Create)
> SetFullPerformanceModeEnabled(...) (in browser.__init__)
> 
> After:
> 
> SetFullPerformanceModeEnabled(...) (in shared_state.__init__)
> ClearCaches() (in desktop_browser_finder.Create, in shared_state.WillRunStory)
> 
> Hence with this CL, the relative order of these two events would be back to what they were before
> the regression in crbug/792357
> 
> Bug:  chromium:792860 
> Bug: chromium:792357
> Change-Id: Ibeba9ca0af55921514d4f3fecf692c3a740b1aac
> Reviewed-on: https://chromium-review.googlesource.com/814214
> Commit-Queue: Ned Nguyen <nednguyen@google.com>
> Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

TBR=perezju@chromium.org,nednguyen@google.com

Change-Id: I0dabdb40acfe646d579fb14ae95cf752a140fcbd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:792860 , chromium:792357
Reviewed-on: https://chromium-review.googlesource.com/816659
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475/telemetry/telemetry/core/platform.py
[modify] https://crrev.com/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475/telemetry/telemetry/internal/platform/platform_backend.py
[modify] https://crrev.com/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475/telemetry/telemetry/page/shared_page_state.py
[modify] https://crrev.com/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475/telemetry/telemetry/internal/browser/browser.py
[modify] https://crrev.com/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475/telemetry/telemetry/testing/fakes/__init__.py
[modify] https://crrev.com/3572e89d92c2ce8d58ab62ad7fde5e2f628c1475/telemetry/telemetry/internal/browser/browser_unittest.py

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9

commit f9663d1bd9be20b9277dfda0b5b23d6dda6951c9
Author: Nghia Nguyen <nednguyen@google.com>
Date: Tue Dec 19 12:47:49 2017

Remove platform_backend's DidStartBrowser and WillCloseBrowser hooks

SetFullPerformanceModeEnabled(False) is now called after all the test has run
in SharedPageState.TearDownState() instead of relying browser objects tracking
which is fragile & complex.


This is a partial reland of https://chromium-review.googlesource.com/c/catapult/+/814214


Bug:  chromium:792860 
Bug: chromium:792357
Bug:  chromium:793050 
Change-Id: I0e4ec50230ccd32c47c34b92cd30edeae7322edf
Reviewed-on: https://chromium-review.googlesource.com/833179
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9/telemetry/telemetry/core/platform.py
[modify] https://crrev.com/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9/telemetry/telemetry/internal/platform/platform_backend.py
[modify] https://crrev.com/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9/telemetry/telemetry/page/shared_page_state.py
[modify] https://crrev.com/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9/telemetry/telemetry/internal/browser/browser.py
[modify] https://crrev.com/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9/telemetry/telemetry/testing/fakes/__init__.py
[modify] https://crrev.com/f9663d1bd9be20b9277dfda0b5b23d6dda6951c9/telemetry/telemetry/internal/browser/browser_unittest.py

Status: Fixed (was: Untriaged)
Blocking: 784319

Comment 6 by benhenry@google.com, Jan 16 (6 days ago)

Components: Test>Telemetry

Comment 7 by benhenry@google.com, Jan 16 (6 days ago)

Components: -Speed>Telemetry

Sign in to add a comment