Bug 70714 – [chromium] Encapsulate state machine parts of scheduling in CCSchedulerStateMachine

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Encapsulate state machine parts of scheduling in CCSchedulerStateMachine

[Nat Duca]

Created attachment 112163 [details]
Patch

[Nat Duca]

This patch introduces the CCSchedulerStateMachine as a place to store the state machine that we typically keep inside render_widget. Goal #1 here was getting our scheduling algorithms in one place and well-covered by tests. Its integration with the rest of code will be a followup patch.

The first thing to notice in this code is how "actions" are taken by the scheduler. The obvious way to do this would be to have
  CCSchedulerClient {
     void draw();
  }
And in the code that calls draw, we'd write:
    if ( ... conditions leading to draw ...)
      m_client->draw();
       ... clear conditions that indicate drawing needed

The thing that has always caused me pain with such code is that it commingles "deciding what to do" with "advancing state." So, I switched to an "ACTION" model:
   Action getNextState() {
        if (... need to draw...)
           return draw;
   }
   void updateState(Action) {
       if (draw)
          clear conditions that lead to draw;
   }
This may have been a mistake. I'm open to feedback.

One key challenge was whether "we are on a vsync line" was state or not. Since the scheduler cannot directly affect vsync, I opted for it being ephemeral state: when one asks for a next state, you pass in whether you're on the vsync. The idea is that b70713's FrameRateController will be the source of that information.

Putting this all together, the rough way one uses the CCSchedulerStateMachine is:
  m_ssm.updateState(... something that we did outside the scheduler ...)
  reschedule();
I.e. anytime you tweak the scheduler state, reschedule.
Where:
  void reschedule() {
      while(true) {
          Action action = m_ssm->getNextAction();
          if (action == ACTION_NONE) break;
          handleAction(action);
      }
  }
  void handleAction(Action action) {
     if(action == DRAW) { drawLayersAndSwap(); }
     if(action == BEGIN_FRAME) { beginFrame(); }
     ...
     m_ssm->updateState(action);
  }


The last few tests in CCSchedulerStateMachineTest.cpp show how a frame runs through the machine. As a result, they might be a good place to start when looking through the code.

Have at it! :)

[James Robinson]

Comment on attachment 112163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112163&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:33
> +    if (m_commitState == COMMIT_STATE_IDLE) {

would this be clearer as a switch statement? one advantage of switching on an enum value is that the compiler will yell if we're missing a case: instead of relying on a runtime ASSERT_NOT_REACHED()

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:46
> +        if (insideVSyncTick && m_needsRedraw)

nit: the other conditionals are (m_dlkfj && insideVSyncTick), why's this one the other way around?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:61
> +    if (action == ACTION_NONE)

switch maybe?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:71
> +        // Prevent another UPDATE_MORE_RESOURCES call until we draw again.

I can't really tell what an "UPDATE_MORE_RESOURCES call" is in this context

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:93
> +    if (input == INPUT_SET_NEEDS_REDRAW) {

switch?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:31
> +/**
> + * The CCSchedulerStateMachine decides how to coordinates main thread activites

why is this a c-style comment? we normally only use that for license header blocks

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:39
> + * The scheduler seprates "what to do next" from the updating of its internal state to

seprates -> separates

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:78
> +    Action getNextAction(bool insideVSyncTick) const;

we would normally name this 'nextAction()' - or if you want to emphasize that this isn't just a simple getter maybe calculateNextAction()?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:79
> +    void updateState(Action);

can you add some comments here (or possibly at the class-level comment) about how a caller is supposed to use these two functions?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:82
> +        INPUT_SET_NEEDS_REDRAW = 1,

the other enums start at 0, why does this one start at 1?

do you need to provide explicit numerical values for these or could you just let the compiler do that?

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:45
> +/* Exposes the protected state fields of the CCSchedulerStateMachine for testing */

use a // style comment for 1-liners

[James Robinson]

Comment on attachment 112163 [details]
Patch

R- for pile of nits to remove this from pending-review queue, although this seems great overall.

[Nat Duca]

(In reply to comment #4)
>  do you need to provide explicit numerical values for these or could you just let the compiler do that?

When gtests fail, they give a number. It helped my head debug those failures rather than having to count enums to recreate what the compiler did in my head. Its style I guess, I can yank it?

[Nat Duca]

Created attachment 112448 [details]
input enum -> real functions

[James Robinson]

I don't think there's anything horribly wrong with giving enums explicit values, but it isn't what we normally do unless the values are going to be used in a bitfield or the numeric values are part of the public API.  Seeing numeric values in the header makes me think that one of those two things is going to happen.

When debugging I can't remember finding explicit numeric values in the source very useful, since GDB can print out the numeric values and it's not too hard to count when looking at logs or other non-interactive debugging modes.

[James Robinson]

Comment on attachment 112448 [details]
input enum -> real functions

View in context: https://bugs.webkit.org/attachment.cgi?id=112448&action=review

R=me, remaining nits are up to you

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:43
> +            : m_commitState(COMMIT_STATE_IDLE)

odd indentation - the : should be 4 spaces in from the previous line. Could you move the c'tor to the .cpp? there's a whole lotta lines to read here before we get to the API bits that a user of this class would be interested in

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:180
> +TEST(CCSchedulerStateMachineTest, TestSetNeedsCommitIsntLost)

IsntLost->IsNotLost ? looks like a typo without the apostrophe (although you obviously can't put that in the function name)

[Nat Duca]

Agreed with all remaining nits. Will land tomorrow morning.

[Nat Duca]

Committed r98612: <http://trac.webkit.org/changeset/98612>

[Nat Duca]

Reverted r98612 for reason:

Broke build

Committed r98613: <http://trac.webkit.org/changeset/98613>

[Nat Duca]

Committed r98618: <http://trac.webkit.org/changeset/98618>