Bug 69049 - [chromium] Extract scheduling logic from CCThreadProxy to its own class
Summary: [chromium] Extract scheduling logic from CCThreadProxy to its own class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-28 17:59 PDT by James Robinson
Modified: 2011-10-04 15:37 PDT (History)
7 users (show)

See Also:


Attachments
Patch (40.71 KB, patch)
2011-09-28 18:06 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (36.61 KB, patch)
2011-09-29 19:30 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (36.57 KB, patch)
2011-09-30 14:13 PDT, James Robinson
no flags Details | Formatted Diff | Diff
merged up against ToT (38.42 KB, patch)
2011-10-03 13:50 PDT, James Robinson
no flags Details | Formatted Diff | Diff
sort gypi entries (39.22 KB, patch)
2011-10-03 13:51 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-09-28 17:59:47 PDT
[chromium] Extract scheduling logic from CCThreadProxy to its own class
Comment 1 James Robinson 2011-09-28 18:06:34 PDT
Created attachment 109107 [details]
Patch
Comment 2 Kenneth Russell 2011-09-29 08:44:42 PDT
Comment on attachment 109107 [details]
Patch

Looks like this patch needs to be merged against TOT. Could enne or nduca do an unofficial review?
Comment 3 James Robinson 2011-09-29 08:54:28 PDT
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.
Comment 4 James Robinson 2011-09-29 19:30:23 PDT
Created attachment 109241 [details]
Patch
Comment 5 James Robinson 2011-09-29 19:32:11 PDT
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.
Comment 6 James Robinson 2011-09-29 19:32:48 PDT
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.
Comment 7 James Robinson 2011-09-30 14:13:37 PDT
Created attachment 109343 [details]
Patch
Comment 8 Kenneth Russell 2011-10-03 10:47:52 PDT
Requesting again that enne, nduca or vangelis do the unofficial review of this change.
Comment 9 Shawn Singh 2011-10-03 12:46:20 PDT
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?
Comment 10 James Robinson 2011-10-03 12:58:12 PDT
(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?
Comment 11 Shawn Singh 2011-10-03 13:32:33 PDT
(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.
Comment 12 Shawn Singh 2011-10-03 13:34:17 PDT
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.
Comment 13 James Robinson 2011-10-03 13:50:10 PDT
(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).
Comment 14 James Robinson 2011-10-03 13:50:32 PDT
Created attachment 109525 [details]
merged up against ToT
Comment 15 James Robinson 2011-10-03 13:51:44 PDT
Created attachment 109526 [details]
sort gypi entries
Comment 16 Vangelis Kokkevis 2011-10-04 14:46:52 PDT
Comment on attachment 109526 [details]
sort gypi entries

This all looks reasonable to me. Unofficial r+ .
Comment 17 Kenneth Russell 2011-10-04 15:08:35 PDT
Comment on attachment 109526 [details]
sort gypi entries

Thanks. rs=me
Comment 18 WebKit Review Bot 2011-10-04 15:37:33 PDT
Comment on attachment 109526 [details]
sort gypi entries

Clearing flags on attachment: 109526

Committed r96655: <http://trac.webkit.org/changeset/96655>
Comment 19 WebKit Review Bot 2011-10-04 15:37:39 PDT
All reviewed patches have been landed.  Closing bug.