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

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


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

android-webview-nexus6

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

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

Hi khushalsagar@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 : Khushal
  Commit : c3b3a5fd6326109faff86d3f6750629f0369541d
  Date   : Fri Oct 27 23:40:10 2017
  Subject: cc: Perform impl-side invalidations at the beginning of impl frame.

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : power.typical_10_mobile
  Metric       : application_energy_consumption_mwh/application_energy_consumption_mwh
  Change       : 8.77% | 2.89345420889 -> 3.17796043667

Revision             Result                    N
chromium@512310      2.89345 +- 0.126809       9      good
chromium@512339      2.92334 +- 0.249587       9      good
chromium@512346      2.93012 +- 0.0921717      6      good
chromium@512350      2.88653 +- 0.0818711      6      good
chromium@512351      3.25474 +- 0.210979       6      bad       <--
chromium@512352      3.21946 +- 0.143935       6      bad
chromium@512353      3.21251 +- 0.141933       6      bad
chromium@512367      3.17796 +- 0.267043       9      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests power.typical_10_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8964184229445761792


For feedback, file a bug with component Speed>Bisection

+more scheduler folks to help better understand this.

The only test case affected is http___siriuslymeg.tumblr.com_, which definitely had a significant impact (~4 -> ~8). The change above affects rendering of animated images and the test case has multiple instances of it. The test starts with an idle period of 20s, when the animated image is in the visible viewport and you can see constant frames, it scrolls and then goes through another idle period. At this point no animated image is visible so the renderer is not producing frames anymore.

There was an equivalent drop when image animations were enabled on the perf bots (r506969). That change could have had an adverse impact on the animation frame rate since the raster work for it was being deferred until the impl frame deadline, which the change above fixed. I ran this test case through smoothness and thread_times telemetry benchmarks to get a better sense of how it impacted the frame rate and cpu usage.

Looking at fps from Android SurfaceFlinger, main thread animations were at 58.33, impl animations now are at 58.67 and with the patch above reverted its 58. So that's good. Thread times reports more overall cpu time per frame though, but its not on the fast path threads, which is what I assume would get affected if we were doing more work for the animation now. There is an increase in thread_other (0.57 to 0.8) and thread_renderer_main (7.3 to 7.58). Looked at a trace and I see the main thread a lot more busy in general with impl side animation, not doing main frames but waking up frequently from DOMTimer/RendererScheduler even during idle periods while the compositor thread is completely idle. With the patch above, the overall main thread cpu time is slightly more during the interaction (6689 -> 6749ms). And this work doesn't happen in the case of main thread driven animations at all.

Interesting. I'm able to get the same behaviour if I disable compositor image animations and remove the animation timer in BitmapImage in blink. Obviously this means that these images don't animate anymore. But the absence of a timer on the compositor task queue is clearly tickling some code path in the renderer scheduler that's causing it to repeatedly schedule some other work.

Issue 782791 has been merged into this issue.

Issue 782791 has been merged into this issue.

Turns out there is a much simpler explanation for what went wrong. Note to self: If the bug is webview only, test webview not chrome.

The impl side invalidation before this change performed invalidations only during the deadline phase, which we never hit with webview scheduling. We get a BeginImplFrame which puts us in BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME, perform the actions and immediately go to BEGIN_IMPL_FRAME_STATE_IDLE. And then the draw happens via LayerTreeFrameSink whenever it pulls for a frame. So the invalidations were never performed and the animation stalls. As it happens, not animating things saves a lot of battery, which explains the improvement when the feature was enabled on perf bots.

After the change above, we invalidate during the impl frame if no main frame request is pending, which restores the initial behaviour and causes the regression.

I have filed a bug for adding a LayerTreeTest to run with synchronous scheduling and test these animations (786253). I also noticed something for thread_times.key_idle_power_cases, for the current case of main thread driven animations we don't pause the animation during the idle period. Filed 786256 to investigate this.

To clarify further about what I've understood with webview scheduling and ensure my understanding is correct.

We do enter the deadline phase in webview but during DrawForLayerTreeFrameSink and in order to get a draw you have to InvalidateLayerTreeFrameSink. So I think the situation we were getting into is as follows:

1) The animation wants to advance and it requests an invalidation. This triggers a BeginImplFrame.

2) During BeginImplFrame we don't perform an invalidation because we want to wait until the draw deadline, so no action is taken and no draw is requested.

3) Since no draw is requested, we never enter the deadline phase and the invalidation is never performed.

This got fixed with this change because now we do the invalidation during BeginImplFrame which creates a pending tree and as a result causes a draw. This also makes me wonder if we should be thinking about a different deadline for these invalidations in the webview case. If the main thread stalls for a long time, we could easily animate on the impl thread. With the default scheduling, this would happen during the draw deadline controlled by cc scheduler. But with synchronous scheduling, we would keep waiting on the main thread.

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

commit 30d6474b8320c8f48d444357e2268be313efc6b0
Author: Khushal <khushalsagar@chromium.org>
Date: Tue Nov 21 00:01:25 2017

cc: Add test for image animations with synchronous scheduling.

Add support to simulate synchronous scheduling in LayerTreeTests and
ensure image animations are tested with this mode.

R=enne@chromium.org, sunnyps@chromium.org

Bug: 786253,  780240 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I3096c89be2ac34e002b3607e52398cb012b28fde
Reviewed-on: https://chromium-review.googlesource.com/780060
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518003}
[modify] https://crrev.com/30d6474b8320c8f48d444357e2268be313efc6b0/cc/test/layer_tree_test.cc
[modify] https://crrev.com/30d6474b8320c8f48d444357e2268be313efc6b0/cc/trees/layer_tree_host_unittest.cc