Bug 69049 – [chromium] Extract scheduling logic from CCThreadProxy to its own class

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

[James Robinson]

[chromium] Extract scheduling logic from CCThreadProxy to its own class

[James Robinson]

Created attachment 109107 [details]
Patch

[Kenneth Russell]

Comment on attachment 109107 [details]
Patch

Looks like this patch needs to be merged against TOT. Could enne or nduca do an unofficial review?

[James Robinson]

I'll merge and post a new patch - I also think that upon reflection I'm not doing the commit-then-redraw stuff correctly, so I'll revisit that as well.

[James Robinson]

Created attachment 109241 [details]
Patch

[James Robinson]

Updated and moved the commitThenRedraw logic to CCLayerTreeHost, which is where I think it really belongs. The behavior now is that calling commitThenRedraw requests a commit and then sets a bit on the main thread side such that after the next commit a redraw is requested, regardless of what's happened on the thread side.  This is what the callsites really intend and is simpler to boot.

One side effect of this is that now CCScheduler has basically no logic at all and the tests are pretty tautological, but I think that's an OK problem to have - we can expand this when we add more sophisticated scheduling logic with the infrastructure already in place to test it, etc.

[James Robinson]

Another nice effect of this patch is that with it applied you can run the layout tests in threaded mode without hitting the ASSERT(!m_beginFram...) in finishAllRenderingOnCCThread() on every single test.

[James Robinson]

Created attachment 109343 [details]
Patch

[Kenneth Russell]

Requesting again that enne, nduca or vangelis do the unofficial review of this change.

[Shawn Singh]

Comment on attachment 109343 [details]
Patch

I'm not qualified to fully review this patch (much less no one asked me to =) ), but I did have some minor comments, particularly on the tests.

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:56
> +    void didCommit();
> +    void didDraw();

Would it be OK to re-name these with a more command-like verb phrase?   At first I thought these were boolean accessors.  What about something like "resetPendingCommit" ?

> Source/WebKit/chromium/tests/CCSchedulerTest.cpp:69
> +    m_scheduler->requestCommit();
> +    m_scheduler->requestCommit();

If we want to make sure that requestCommit() calls scheduleBeginFrameAndCommit() on the first call, maybe we should add another EXPECT_CALL(...).Times(0) in-between these two calls to requestCommit().

Also, though its not technically necessary, in my tests I felt more comfortable explicitly using Mock::VerifyAndClearExpectations, both for readability and safety... especially if we use .Times(0).  The semantics of EXPECT_CALL does not "retire" previous expectations.

> Source/WebKit/chromium/tests/CCSchedulerTest.cpp:73
> +    m_scheduler->didCommit();
> +    m_scheduler->requestCommit();

The same thing might apply here, too.  Maybe we could just move the EXPECT_CALL one line down?

[James Robinson]

(In reply to comment #9)
> > Source/WebKit/chromium/tests/CCSchedulerTest.cpp:69
> > +    m_scheduler->requestCommit();
> > +    m_scheduler->requestCommit();
> 
> If we want to make sure that requestCommit() calls scheduleBeginFrameAndCommit() on the first call, maybe we should add another EXPECT_CALL(...).Times(0) in-between these two calls to requestCommit().

I'm trying to assert that I get exactly one call to scheduleBeginFrameAndCommit() in response to these two calls - and from local testing I think that's the behavior I was getting.  I'd consider this section failing if I got 0 or 2+ calls, but beyond that I'm not trying to be too precise - it's not part of the test whether the first call or the second call yields the sBFAC(), for example.

> 
> Also, though its not technically necessary, in my tests I felt more comfortable explicitly using Mock::VerifyAndClearExpectations, both for readability and safety... especially if we use .Times(0).  The semantics of EXPECT_CALL does not "retire" previous expectations.
> 

I'm not sure I fully grok the EXPECT_CALL semantics here - the code I wrote is based mostly off of local experiments, so it's possible I'm not doing it correctly.  I find the retiring logic for the mock expectations to be pretty confusing (or maybe it's just the docs).

I'm not sure when exactly to use Mock::VerifyAndClearExpectations().  Does that cause the test to immediately fail if there are unfulfilled expectations at that point?  The fact that it only shows up in the CookBook and not in the general tutorials makes me wonder if it's supposed to be used all the time.  When I set new expectations on a mock does that action cause it to verify previously set expectations are already fulfilled?

[Shawn Singh]

(In reply to comment #10)
> (In reply to comment #9)
> > > Source/WebKit/chromium/tests/CCSchedulerTest.cpp:69
> > > +    m_scheduler->requestCommit();
> > > +    m_scheduler->requestCommit();
> > 
> > If we want to make sure that requestCommit() calls scheduleBeginFrameAndCommit() on the first call, maybe we should add another EXPECT_CALL(...).Times(0) in-between these two calls to requestCommit().
> 
> I'm trying to assert that I get exactly one call to scheduleBeginFrameAndCommit() in response to these two calls - and from local testing I think that's the behavior I was getting.  I'd consider this section failing if I got 0 or 2+ calls, but beyond that I'm not trying to be too precise - it's not part of the test whether the first call or the second call yields the sBFAC(), for example.
> 
> > 
> > Also, though its not technically necessary, in my tests I felt more comfortable explicitly using Mock::VerifyAndClearExpectations, both for readability and safety... especially if we use .Times(0).  The semantics of EXPECT_CALL does not "retire" previous expectations.
> > 
> 
> I'm not sure I fully grok the EXPECT_CALL semantics here - the code I wrote is based mostly off of local experiments, so it's possible I'm not doing it correctly.  I find the retiring logic for the mock expectations to be pretty confusing (or maybe it's just the docs).
> 
> I'm not sure when exactly to use Mock::VerifyAndClearExpectations().  Does that cause the test to immediately fail if there are unfulfilled expectations at that point?  The fact that it only shows up in the CookBook and not in the general tutorials makes me wonder if it's supposed to be used all the time.  When I set new expectations on a mock does that action cause it to verify previously set expectations are already fulfilled?

As I understand it, an expectation is "retired" at the destructor, or whenever we explicitly tell it (through  .retiresOnSaturation, or Mock::VerifyAndClearExpectations).   If we specify an expectation on the same function (even if we expect different parameters or behavior), it takes precedence over the older expectation.  I think their primary reason for doing this was to allow "default expectations" to be specified, and then users can override those expectations without knowing about the default.  If their new expectations get retired, it goes back to the default expectations.

However, because of this behavior, its also possible to retire an expectation and accidentally have a previous expectation cause an error (or worse, a cause a false positive).   So, for me personally, to avoid unnecessary "cognitive load" trying to think about it, and to avoid accidental error-prone code, I prefer to always verify and clear expectations before making a new expectation on the same function.  I also prefer to avoid .retiresOnSaturation, because in special cases (like expecting 0 calls) the semantics seem unclear and error-prone.

[Shawn Singh]

Oh and to answer the other part of your question, verifyAndClearExpectations causes the expectation to be verified exactly at that point (as opposed to waiting until the destructor).  If expectations are not met by then, the test case fails.

[James Robinson]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > > Source/WebKit/chromium/tests/CCSchedulerTest.cpp:69
> > > > +    m_scheduler->requestCommit();
> > > > +    m_scheduler->requestCommit();
> > > 
> > > If we want to make sure that requestCommit() calls scheduleBeginFrameAndCommit() on the first call, maybe we should add another EXPECT_CALL(...).Times(0) in-between these two calls to requestCommit().
> > 
> > I'm trying to assert that I get exactly one call to scheduleBeginFrameAndCommit() in response to these two calls - and from local testing I think that's the behavior I was getting.  I'd consider this section failing if I got 0 or 2+ calls, but beyond that I'm not trying to be too precise - it's not part of the test whether the first call or the second call yields the sBFAC(), for example.
> > 
> > > 
> > > Also, though its not technically necessary, in my tests I felt more comfortable explicitly using Mock::VerifyAndClearExpectations, both for readability and safety... especially if we use .Times(0).  The semantics of EXPECT_CALL does not "retire" previous expectations.
> > > 
> > 
> > I'm not sure I fully grok the EXPECT_CALL semantics here - the code I wrote is based mostly off of local experiments, so it's possible I'm not doing it correctly.  I find the retiring logic for the mock expectations to be pretty confusing (or maybe it's just the docs).
> > 
> > I'm not sure when exactly to use Mock::VerifyAndClearExpectations().  Does that cause the test to immediately fail if there are unfulfilled expectations at that point?  The fact that it only shows up in the CookBook and not in the general tutorials makes me wonder if it's supposed to be used all the time.  When I set new expectations on a mock does that action cause it to verify previously set expectations are already fulfilled?
> 
> As I understand it, an expectation is "retired" at the destructor, or whenever we explicitly tell it (through  .retiresOnSaturation, or Mock::VerifyAndClearExpectations).   If we specify an expectation on the same function (even if we expect different parameters or behavior), it takes precedence over the older expectation.  I think their primary reason for doing this was to allow "default expectations" to be specified, and then users can override those expectations without knowing about the default.  If their new expectations get retired, it goes back to the default expectations.
> 

I see.  So there are two possible failures for this case - too many calls or too few.  I've set up a no-call expectation for all mock functions in the test constructor, so if an expectation is satisfied and then the mock is called again, it'll fail immediately because there aren't any valid expectations for it with an error like this:

../../third_party/WebKit/Source/WebKit/chromium/tests/CCSchedulerTest.cpp:67: Failure
Mock function called more times than expected - returning directly.
    Function call: scheduleBeginFrameAndCommit()
         Expected: to be called once
           Actual: called twice - over-saturated and active


But that won't catch the case where we don't get the call we expect.  So the concern is that we'll miss a call, and then later in the same testcase set a new expectation on that mock that retroactively makes the test pass?  I think I get the theory but I can't figure out how to construct this scenario in an actual test - do you know how this would work?

> However, because of this behavior, its also possible to retire an expectation and accidentally have a previous expectation cause an error (or worse, a cause a false positive).   So, for me personally, to avoid unnecessary "cognitive load" trying to think about it, and to avoid accidental error-prone code, I prefer to always verify and clear expectations before making a new expectation on the same function.  I also prefer to avoid .retiresOnSaturation, because in special cases (like expecting 0 calls) the semantics seem unclear and error-prone.

I think it's more important that we understand our tools than to try to second-guess them.  This mocking stuff is definitely pretty particular (and I'm starting to question our use of it if we can't even figure out the basics).

[James Robinson]

Created attachment 109525 [details]
merged up against ToT

[James Robinson]

Created attachment 109526 [details]
sort gypi entries

[Vangelis Kokkevis]

Comment on attachment 109526 [details]
sort gypi entries

This all looks reasonable to me. Unofficial r+ .

[Kenneth Russell]

Comment on attachment 109526 [details]
sort gypi entries

Thanks. rs=me

[WebKit Review Bot]

Comment on attachment 109526 [details]
sort gypi entries

Clearing flags on attachment: 109526

Committed r96655: <http://trac.webkit.org/changeset/96655>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.