|
|
Created:
6 years ago by mithro-old Modified:
5 years, 8 months ago CC:
cc-bugs_chromium.org, chromium-reviews, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Split out BeginFrame needed verse proactive for commits.
In "cc: Only send a BeginMainFrame inside an BeginImplFrame."
https://codereview.chromium.org/798323003/ a commit can only occur inside an
impl frame - this means BeginFrames are *needed* if we want to commit.
We also want to proactively request BeginFrames if a commit is pending (for
performance), but it is not actually required.
BUG=346230
Committed: https://crrev.com/102333b3fdd6e288f1e80f77d745269b405d38b4
Cr-Commit-Position: refs/heads/master@{#324356}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase onto master. #Patch Set 3 : Reworked patch now Sunny has landed the webview refactor. #Patch Set 4 : Renaming methods to be clearer about intentions. #
Total comments: 2
Patch Set 5 : Rebase onto master. #Messages
Total messages: 21 (3 generated)
mithro@mithis.com changed reviewers: + brianderson@chromium.org, sunnyps@chromium.org
Hi, A small fix and test for a case missed in https://codereview.chromium.org/798323003/ Tim
On 2014/12/19 02:29:24, mithro wrote: > Hi, > > A small fix and test for a case missed in > https://codereview.chromium.org/798323003/ > > Tim I'm getting a bunch of flakes on the bot, I'm unsure if it's my fault or if there is something else going on. From win_chromium_x64_rel_ng // win_chromium_rel_ng -- content_browsertests (with patch) content_browsertests (with patch) 122 disabled failed 2 failures: DumpAccessibilityEventsTest.AccessibilityEventsInputTypeTextValueChanged DumpAccessibilityEventsTest.AccessibilityEventsCheckedStateChanged From android_dbg_tests_recipe -- Instrumentation test AndroidWebViewTest Still trying to figure out it. The AndroidWebViewTest seems a bit suspicious. Output is very hard to read though. Tim
https://codereview.chromium.org/792803008/diff/1/cc/scheduler/scheduler_state... File cc/scheduler/scheduler_state_machine.cc (left): https://codereview.chromium.org/792803008/diff/1/cc/scheduler/scheduler_state... cc/scheduler/scheduler_state_machine.cc:441: BeginFrameNeeded()) The check for BeginFrameNeeded is so we can commit activate and be ready to draw before we request a BeginFrame from WebView. I think Sunny's patch is a pre-req to this patch, and is likely why you were seeing WebView test flake: https://codereview.chromium.org/817603002
https://codereview.chromium.org/792803008/diff/1/cc/scheduler/scheduler_state... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/792803008/diff/1/cc/scheduler/scheduler_state... cc/scheduler/scheduler_state_machine.cc:704: BeginFrameNeededForChildren() || ProactiveBeginFrameWanted()); OR needs_commit_ here. https://codereview.chromium.org/792803008/diff/1/cc/scheduler/scheduler_state... cc/scheduler/scheduler_state_machine.cc:760: return needs_animate_ || needs_redraw_ || needs_commit_; Remove needs_commit_ from here.
FYI - the tests I'm trying to make pass in https://codereview.chromium.org/787763006/ are below; ---- C 216.066s Main [ FAILED ] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackFromJavaDuringFullscreenTransitions C 216.066s Main [ FAILED ] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackFromJsDuringFullscreenTransitions C 216.066s Main [ FAILED ] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackWaitsForJs C 216.066s Main [ FAILED ] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackWhenContainerViewDetached ---- C 216.060s Main ******************************************************************************** C 216.061s Main Detailed Logs C 216.061s Main ******************************************************************************** C 216.061s Main [FAIL] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackFromJavaDuringFullscreenTransitions: C 216.061s Main junit.framework.AssertionFailedError C 216.061s Main at org.chromium.android_webview.test.VisualStateTest.testVisualStateCallbackFromJavaDuringFullscreenTransitions(VisualStateTest.java:321) C 216.061s Main at java.lang.reflect.Method.invokeNative(Native Method) C 216.061s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 216.061s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 216.061s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 216.061s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 216.061s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 216.061s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 216.061s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 216.061s Main [FAIL] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackFromJsDuringFullscreenTransitions: C 216.061s Main junit.framework.AssertionFailedError C 216.062s Main at org.chromium.android_webview.test.VisualStateTest.testVisualStateCallbackFromJsDuringFullscreenTransitions(VisualStateTest.java:243) C 216.062s Main at java.lang.reflect.Method.invokeNative(Native Method) C 216.062s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 216.062s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 216.062s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 216.062s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 216.062s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 216.062s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 216.062s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 216.063s Main C 216.063s Main [FAIL] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackWaitsForJs: C 216.063s Main junit.framework.AssertionFailedError C 216.063s Main at org.chromium.android_webview.test.VisualStateTest.testVisualStateCallbackWaitsForJs(VisualStateTest.java:172) C 216.063s Main at java.lang.reflect.Method.invokeNative(Native Method) C 216.063s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 216.063s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 216.063s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 216.063s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 216.064s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 216.064s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 216.064s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) C 216.064s Main C 216.064s Main [FAIL] org.chromium.android_webview.test.VisualStateTest#testVisualStateCallbackWhenContainerViewDetached: C 216.064s Main junit.framework.AssertionFailedError C 216.064s Main at org.chromium.android_webview.test.VisualStateTest.testVisualStateCallbackWhenContainerViewDetached(VisualStateTest.java:382) C 216.064s Main at java.lang.reflect.Method.invokeNative(Native Method) C 216.064s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 216.065s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 216.065s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 216.065s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) C 216.065s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) C 216.065s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) C 216.065s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701)
Tim, any idea how to work around the WebView monotonicity issues? It might be a while before Sunny's patch lands and a work around might be preferable.
Hi everyone, This patch fixes a small "schematic" issue with the way commits and BeginFrame worked. This patch has not actual functionally effect because currently ProactiveBeginFrameWanted() is always used as part of BeginFrameNeeded. However, if we were to change the behaviour so that ProactiveBF was optional the current version would break and it would be very unclear why. This is the remnants of the previous patch after Sunny's webview CL refactor landed and splitting the test change out to https://codereview.chromium.org/1055673005/ Can you please take a look? Tim 'mithro' Ansell
That should have been "semantic" not "schematic".
Are any tests affected by this change?
On 2015/04/07 03:29:52, brianderson wrote: > Are any tests affected by this change? This does not change actual behaviour at all, it only affects the meaning to a human.
I don't understand why this change is being made. It's really confusing to have to look at commit related state in two places and reason about this. I think a better change to make would be to consolidate all the BeginFrameNeeded... methods into a single method rather than move things around. Then one could look at that single method and read through it and understand what's happening.
On 2015/04/07 at 23:24:27, sunnyps wrote: > I don't understand why this change is being made. It's really confusing to have to look at commit related state in two places and reason about this. > > I think a better change to make would be to consolidate all the BeginFrameNeeded... methods into a single method rather than move things around. Then one could look at that single method and read through it and understand what's happening. On 2015/04/07 23:24:27, sunnyps wrote: > I don't understand why this change is being made. It's really confusing to have > to look at commit related state in two places and reason about this. Because there are two different concepts here; * BeginFrames begin *required*. * BeginFrames begin proactively requested to help with BeginFrame messages not being instant. The system should perform correctly if the proactive requests stop occurring, performance will just be less optimal. I guess we should add tests which check that this actually is still true. Does that make sense? > I think a better change to make would be to consolidate all the > BeginFrameNeeded... methods into a single method rather than move things around. > Then one could look at that single method and read through it and understand > what's happening. I've uploaded a new version which renames these methods to try and make it clearer the distinction. Tim 'mithro' Ansell
I guess what I'm trying to say is that the proactive vs non-proactive distinction existed only because of special behavior for WebView. If BeginFrameNeededToAnimateOrDraw returned true it was guaranteed that the upcoming BeginFrame would cause a draw to happen. I don't think the distinction makes a whole lot of sense now that all platforms behave in a similar way.
lgtm. I like keeping the distinction between required vs. nice-to-have BeginFrames so that the intention of the code is easier to understand and modify. If we do combine the two functions, it would be nice to add a comment separating the two types of checks - but I slightly prefer separate functions still. https://codereview.chromium.org/792803008/diff/60001/cc/scheduler/scheduler_s... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/792803008/diff/60001/cc/scheduler/scheduler_s... cc/scheduler/scheduler_state_machine.cc:727: return (BeginFrameRequiredForAction() || BeginFrameRequiredForChildren() || Should we put BeginFrameRequiredForChildren above the HasInitializedOutputSurface check? None of our BeginFrameSources are tied to the existence of an OutputSurface anymore.
The CQ bit was checked by mithro@mithis.com
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/792803008/#ps80001 (title: "Rebase onto master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/792803008/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/102333b3fdd6e288f1e80f77d745269b405d38b4 Cr-Commit-Position: refs/heads/master@{#324356}
Message was sent while issue was closed.
https://codereview.chromium.org/792803008/diff/60001/cc/scheduler/scheduler_s... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/792803008/diff/60001/cc/scheduler/scheduler_s... cc/scheduler/scheduler_state_machine.cc:727: return (BeginFrameRequiredForAction() || BeginFrameRequiredForChildren() || On 2015/04/08 01:46:56, brianderson wrote: > Should we put BeginFrameRequiredForChildren above the > HasInitializedOutputSurface check? None of our BeginFrameSources are tied to > the existence of an OutputSurface anymore. I'll do that in a different CL. |