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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg-tG_5wsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg-sPygAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDghrDcsQkM


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

chromium-rel-win7-gpu-intel
chromium-rel-win8-dual

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978194628361125680

=== Auto-CCing suspected CL author eseckler@chromium.org ===

Hi eseckler@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : eseckler
  Commit : e356c64f253923983248656d304d8aa471aad5f0
  Date   : Fri May 26 12:59:36 2017
  Subject: [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler.

Bisect Details
  Configuration: win_8_perf_bisect
  Benchmark    : performance_browser_tests
  Metric       : CastV2Performance_gpu_30fps_slow/video_jitter
  Change       : 123.25% | 0.502985666667 -> 1.1229075

Revision             Result                    N
chromium@474975      0.502986 +- 0.281391      6      good
chromium@474983      0.493595 +- 0.28248       6      good
chromium@474987      0.631624 +- 0.348938      6      good
chromium@474988      0.509047 +- 0.435171      6      good
chromium@474989      1.07529 +- 0.111482       6      bad       <--
chromium@474990      1.14284 +- 0.258005       6      bad
chromium@475005      1.20162 +- 0.110381       6      bad
chromium@475035      1.12142 +- 0.137433       6      bad
chromium@475095      1.12291 +- 0.236591       6      bad

To Run This Test
  .\src\out\Release\performance_browser_tests.exe --test-launcher-print-test-stdio=always --enable-gpu

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8978194628361125680

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6172130548383744


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

 Issue 727635  has been merged into this issue.

The regressions in performance_browser_tests seem to be due to the removal of "Waiting for damage from root surface" deadlines: We are now waiting until the normal deadline if we know the root surface is trying to produce damage, too, whereas previously we were waiting for a slightly shorter deadline if only the root surface damage was missing [1].

In these particular tests, a RenderWidget and the browser compositor are producing CompositorFrames simultaneously. It just so happens that waiting slightly less long for the root surface is beneficial for video frame jitter in these tests - and on these particular machines -, because the timing works out in such a way that we skip less frames due to deciding to draw (without corresponding root surface damage) earlier. See attached traces.

I'm not sure how to best combat this - Maybe this is a regression worth accepting? Alternatively, we could reintroduce the special "Waiting for damage from root surface" deadlines - yet, I'm not sure whether this would be overall beneficial or only for this particular synthetic test.

[1] https://codereview.chromium.org/2854163003/diff/180001/cc/surfaces/display_scheduler.cc

Deleted: trace_bot_without_displayschedacks.json.gz 2.1 MB

trace_bot_without_displayschedacks.json.gz 2.1 MB Download

Deleted: trace_bot_with_displayschedacks.json.gz 2.1 MB

trace_bot_with_displayschedacks.json.gz 2.1 MB Download

And another trace with all default categories. (I'm still not entirely sure why the browser compositor is producing frames during the first few seconds of the test - it looks like something may be animating on it.)

Deleted: trace_bot_with_displayschedacks_allcategories.json.gz 7.8 MB

trace_bot_with_displayschedacks_allcategories.json.gz 7.8 MB Download

The browser compositor seems to be drawing for the first 5 secs or so in each test.

TBH the trace without acks does look better. I don't know how to proceed on this. Maybe our regular deadline is too late and we can tweak that instead of adding a workaround for the root surface?

At the very least we should fix the test so that it doesn't animate browser UI unnecessarily. (Maybe check if it's running with the --disable-fre flag first).

Figured out what's animating: When casting is first activated, a cast logo is shown on the browser tab, which animates its opacity for ~5sec (fading in and out a few times).

So I'm guessing that this browser-side animation is intended and probably not easy to deactivate.

Regarding the regular deadline: We can try to tweak this, but risk regressing other tests / use cases, where waiting longer is beneficial because e.g. a renderer is slower and cc::display draw is quick.

The reason it's a problem here seems to be a combination of custom animation + proactive BeginFrame in cc::Scheduler. First, the animation (in the browser) uses a custom timer (in gfx::AnimationContainer) rather than a compositor animation - and this timer may fire rather late during the BeginFrame interval. Thus, browser-side compositing work isn't well aligned with BeginFrames in the first place. Second, after such an animation, the browser's cc::Scheduler requests a proactive BeginFrame and blocks it in late deadline mode, even though there's no request for a new main frame. The animation ends up not firing at all during this BeginFrame, and thus the DisplayScheduler's deadline (and the renderer's frame) is delayed to the regular deadline.

The first behavior isn't new, the second is now slightly different with acks (because we use the regular rather than root-surface-only deadline).

(Side note, this proactive BeginFrame + late deadline behavior is actually also problematic for headless plans, because it won't work if deadline/interval is infinite. Maybe we'll have to disable that at least for the flush_pipe mode.)

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977264013802294128

=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : eseckler
  Commit : e356c64f253923983248656d304d8aa471aad5f0
  Date   : Fri May 26 12:59:36 2017
  Subject: [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler.

Bisect Details
  Configuration: android_nexus6_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:chrome:all_processes:reported_by_os:system_memory:java_heap:proportional_resident_size_avg/load_games/load_games_spychase
  Change       : 2.96% | 19519658.6667 -> 20098389.3333

Revision             Result                  N
chromium@474979      19519659 +- 353511      6      good
chromium@474986      19663019 +- 221134      6      good
chromium@474988      19608064 +- 148065      6      good
chromium@474989      20120405 +- 207993      6      bad       <--
chromium@474990      20112384 +- 236932      6      bad
chromium@474993      20101632 +- 258656      6      bad
chromium@475007      20182357 +- 384093      6      bad
chromium@475035      20081323 +- 270300      6      bad
chromium@475091      20129109 +- 448474      6      bad
chromium@475202      20098389 +- 220389      6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.games.spychase system_health.memory_mobile

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8977264013802294128

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5821906869026816


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

 Issue 727466  has been merged into this issue.

+miu, owner of performance_browser_tests: should we accept the regression?

miu, any response to #8?

eseckler, any update?

From my side, there's no update here. Explanations in #5 and #8 are still accurate.

The regression is confined to the first 5sec of casting, when the cast icon animates. It's caused by heuristically chosen deadlines, which happen to work slightly less well for this particular test setup (30fps video, on these machines). It's highly dependent on the test setup, e.g. on the duration it takes our bots to draw a video frame. We can tune the heuristics for this test setup by moving the DisplayScheduler's regular deadline earlier, but that risks regressing elsewhere, because we would give damage producers less time.

Maybe it would make sense to adjust the DisplayScheduler's regular deadline dynamically depending on a history of draw times (similar to what cc::Scheduler does), instead of hard-coding it to two thirds of the frame. Sunny, wdyt?

Ah! The throbbing of the indicator icon: This makes sense. I suppose it's fine to accept the regression as a part of normal "startup jank." Perhaps I should adjust performance_browser_tests to omit data during the first 5 seconds, to get smoother, more reliable results? (It's the sustained long-term performance that really matters for, say, video watching.)