All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=792357

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=82ddc3b248367b6ac06db8b5e0c29f29eacc32275d11aaa6bfc7fe6494c4cfff


Bot(s) for this bug's original alert(s):

chromium-rel-win8-dual

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16ae868c040000

📍 Found significant differences after each of 2 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/16ae868c040000

[Telemetry] Cleanup Setup/Collect of browser profile
By perezju@google.com · Thu Nov 23 08:10:56 2017
catapult @ 0bb3c037e45b56f163b2afe1aa3bd1c3e4b542f4

Fix startup browser breakage when clear_sytem_cache_for_browser_and_profile_on_start is set
By nednguyen@google.com · Fri Dec 01 23:11:27 2017
catapult @ c74f9a8fd099ff475505906f6c518f59d0baa322

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Uhh, this is pretty! Thank +dtu for my first pinpoint assigned bug 🍾

Now, trying to understand the regression, I think the relevant CLs probably are:

0bb3c03 [Telemetry] Cleanup Setup/Collect of browser profile (perezju) -> broke "SetProfile"
611930b Move cache flushing logic out of Browser constructor into PossibleBrowser classes (nednguyen) -> broke "ClearCaches"
94594c1 [Telemetry] Fix error resolving user profile directory (perezju) -> fixed "SetProfile"
c74f9a8 Fix startup browser breakage when clear_sytem_cache_for_browser_and_profile_on_start is set (nednguyen) -> fixed "ClearCaches"

Now, behaviorally, 94594c1 should be pretty much a revert of 0bb3c03. While between 611930b and c74f9a8 the only relevant change I can see was the moving of "ClearCaches"

- before: 
   [in browser.__init__] platform_backend.DidCreateBrowser()
   [in browser.__init__] ClearCaches [ inlined code ]

- after:
   [in desktop_browser_finder.Create] browser_backend.ClearCaches()
   [in browser.__init__] platform_backend.DidCreateBrowser()

Also DidCreateBrowser just calls self.SetFullPerformanceModeEnabled(...)

So we swapped the order of those two.

Not sure if there is a preference of which one should come first. Also not sure why the call to SetFullPerformanceModeEnabled is hidden away in "DidCreateBrowser". Maybe should be fine to kill
that method and explicitly SetFullPerformanceModeEnabled in x_browser_finder.Create?

Anyway, whatever we decide, the "regression" sounds like just an artifact of moving around some of these calls. And I don't see how one is necessarily better than the other.

Ned, any thoughts?

I actually kinda suspect that something else regressed memory.long_running_idle_gmail_tbmv2 while we leave the benchmark broken. In no way I can see how swapping ClearCaches & SetFullPerformanceModeEnabled can cause the regression :-/

On another note, I filed  issue 792860  to clean up DidCreateBrowser stuff

While working on  issue 792860 , maybe you can reverse the order of those two statements again, and help to (dis-)prove my theory?

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

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

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