Bug 89035 – Add flushes to CCTextureUpdater::update

• Component: New Bugs
• Status: RESOLVED FIXED

[Brian Anderson]

Add flushes to CCTextureUpdater::update

[Brian Anderson]

Created attachment 147407 [details]
Patch

[WebKit Review Bot]

Attachment 147407 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 147407 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

I think we could have unit tests to verify that we flush at the rate we want to - I can help you figure out how to wrangle these if you want.

[Brian Anderson]

Created attachment 147415 [details]
Patch

[WebKit Review Bot]

Attachment 147415 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Anderson]

Created attachment 147443 [details]
Patch

[WebKit Review Bot]

Attachment 147443 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Comment on attachment 147443 [details]
Patch

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

Now that you have chrome android branch access, take a peek at this file there on clank/m18 branch. Its slightly different, you might be able to leverage some of the thinking there.

Not sure you're getting a lot of value out of having the periodic flush. I think you could just put the flush code inline, its really not that complex.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:97
> +    if ((uploadCount % kUploadFlushPeriod) && context3d)

Ouch that is hard to read. Can you decompose this so that you firs tcheck context3d and early return, then later do the flush?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:-132
> -        GraphicsContext3D* context3d = context->context3D();

why deleted?

[Brian Anderson]

Created attachment 148201 [details]
Patch

[Brian Anderson]

The latest patch adds tests to verify upload/flushing patterns.

>> I think you could just put the flush code inline, its really not that complex.

Forgot to address this, so this patch set doesn't put the flush code inline.  Will address this in the next patch set.  I originally ripped it out into separate functions because the logic was slightly different in the two cases, and I thought naming the cases would make it clearer.

[Brian Anderson]

Some other notes:
The test doesn't address partial uploads.  If we are going to treat partial uploads differently (ie: not flushing as often, like Clank), then I'll need to update the test and flushing logic.

[Brian Anderson]

Created attachment 148421 [details]
Patch

[Brian Anderson]

Both the test and flushing logic are ready for review now.  The both allow the regular texture uploads and the partial texture uploads to have different flush periods.

[Brian Anderson]

Created attachment 148435 [details]
Patch

[Brian Anderson]

The last patch just updates the test combinations.

[Nat Duca]

Comment on attachment 148435 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:92
> +    GraphicsContext3D* context3d = context ? context->context3D() : 0;

why would this be null?

also, why not add flush onto ccgraphicscontext?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:105
> +            if (!(uploadCount % kUploadFlushPeriod))

seems like this context3d check is redundant

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:113
> +        uploadCount = 0;

uploadCount->partialUploadCount

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:133
> +            if (!(uploadCount % kUploadFlushPeriodPartial))

why not just use index here?

also, any reason to not just use the same constant as before?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:138
> +        if (uploadCount % kUploadFlushPeriodPartial)

Whats this flush do?

[Nat Duca]

Also, @epenner found a bug in the scheduler that causes us to do
  scheduledActionUploadMore
  draw
  scheduledAcitonUploadMore

I'm going to cc you on that. Can you please pick up that bug and fix it as a followup to this patch?

[Adrienne Walker]

Comment on attachment 148435 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:92
>> +    GraphicsContext3D* context3d = context ? context->context3D() : 0;
> 
> why would this be null?
> 
> also, why not add flush onto ccgraphicscontext?

It'd be null in the hypothetical-but-not-yet-implemented software mode.  What would flush mean for a software renderer? Would you flush at the same rate?

Given that a software renderer might want to do texture upload throttling differently and the fact that it doesn't exist yet, I'd prefer something like this (which we do in other places, e.g. ManagedTexture.cpp):

if (!context3D) {
    // FIXME: Implement this path for software compositing.
    return;
}

...and then just assuming it's not null everywhere else in this function.

[Nat Duca]

(In reply to comment #18)
> (From update of attachment 148435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148435&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:92
> >> +    GraphicsContext3D* context3d = context ? context->context3D() : 0;
> > 
> > why would this be null?
> > 
> > also, why not add flush onto ccgraphicscontext?
> 
> It'd be null in the hypothetical-but-not-yet-implemented software mode.  What would flush mean for a software renderer? Would you flush at the same rate?

In the software-mode, we'lls till want to flush. The flushing algorithm will be based on fences then so we wont have this constant-based system at all. Net/net, I think flush is still correct.



> Given that a software renderer might want to do texture upload throttling differently and the fact that it doesn't exist yet, I'd prefer something like this (which we do in other places, e.g. ManagedTexture.cpp):
> 
> if (!context3D) {
>     // FIXME: Implement this path for software compositing.
>     return;
> }
> 
> ...and then just assuming it's not null everywhere else in this function.

Sorry, disagree, as noted above.

[Adrienne Walker]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 148435 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=148435&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:92
> > >> +    GraphicsContext3D* context3d = context ? context->context3D() : 0;
> > > 
> > > why would this be null?
> > > 
> > > also, why not add flush onto ccgraphicscontext?
> > 
> > It'd be null in the hypothetical-but-not-yet-implemented software mode.  What would flush mean for a software renderer? Would you flush at the same rate?
> 
> In the software-mode, we'lls till want to flush. The flushing algorithm will be based on fences then so we wont have this constant-based system at all. Net/net, I think flush is still correct.

...which sounds like an entirely different texture uploader implementation?

I don't really feel strongly about this.  It just seemed more reasonable to punt until software mode was more realized.

[Nat Duca]

> ...which sounds like an entirely different texture uploader implementation?
> 
> I don't really feel strongly about this.  It just seemed more reasonable to punt until software mode was more realized.

Agreed. Also, I think the basic rule is "keep it simple and make it work with what we ship today, not what we want to ship in the future."

[Brian Anderson]

Comment on attachment 148435 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:92
>>>>> +    GraphicsContext3D* context3d = context ? context->context3D() : 0;
>>>> 
>>>> why would this be null?
>>>> 
>>>> also, why not add flush onto ccgraphicscontext?
>>> 
>>> It'd be null in the hypothetical-but-not-yet-implemented software mode.  What would flush mean for a software renderer? Would you flush at the same rate?
>>> 
>>> Given that a software renderer might want to do texture upload throttling differently and the fact that it doesn't exist yet, I'd prefer something like this (which we do in other places, e.g. ManagedTexture.cpp):
>>> 
>>> if (!context3D) {
>>>     // FIXME: Implement this path for software compositing.
>>>     return;
>>> }
>>> 
>>> ...and then just assuming it's not null everywhere else in this function.
>> 
>> In the software-mode, we'lls till want to flush. The flushing algorithm will be based on fences then so we wont have this constant-based system at all. Net/net, I think flush is still correct.
> 
> ...which sounds like an entirely different texture uploader implementation?
> 
> I don't really feel strongly about this.  It just seemed more reasonable to punt until software mode was more realized.

Another reason for the null check: Removing the null check causes one or more of the webkit_unit_tests to crash. Should I fix the tests that were passing in a null context instead?

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:105
>> +            if (!(uploadCount % kUploadFlushPeriod))
> 
> seems like this context3d check is redundant

Could be?  I'm not sure though.  I copied the logic from line 133.  Is it possible for "context->contex3D()" to be null when "context" is not null?  If the answer is no, I'll remove the check on line 133 as well.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:113
>> +        uploadCount = 0;
> 
> uploadCount->partialUploadCount

Noted.  Will change.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:133
>> +            if (!(uploadCount % kUploadFlushPeriodPartial))
> 
> why not just use index here?
> 
> also, any reason to not just use the same constant as before?

Good point about using index; it's scope is outside the for loop already so I wouldn't even have to pull it out to check its final value below.

I did not use the same constant because I noticed the Clank version of the patch didn't do any flushes during the partial uploads at all until the end.  I thought using different constants for the period would give us the greatest flexibility for tweaking.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:138
>> +        if (uploadCount % kUploadFlushPeriodPartial)
> 
> Whats this flush do?

If we flush every 4th upload and upload 7 textures, this makes sure that the last three uploads are followed by a flush and aren't "dangling".

[Nat Duca]

(In reply to comment #22)
> (From update of attachment 148435 [details])
> Another reason for the null check: Removing the null check causes one or more of the webkit_unit_tests to crash. Should I fix the tests that were passing in a null context instead?

I understand. Lets try fixin the test or do a git blame to figure out how that original null check got added. Its strange to me to have to null check but if you can convince yourself that its null in production code, then I'm okay. If its test only, then MockWebGraphicsContext is your friend here.

> I did not use the same constant because I noticed the Clank version of the patch didn't do any flushes during the partial uploads at all until the end.  I thought using different constants for the period would give us the greatest flexibility for tweaking.

Ah, I get it. Lets do one value until we need more. We'll need to do a patch later anyway if we want to tweak, and then we'd have hard data to justify the two values.

> If we flush every 4th upload and upload 7 textures, this makes sure that the last three uploads are followed by a flush and aren't "dangling".

Makes sense. Might put a comment explaining.

[Brian Anderson]

Created attachment 150239 [details]
Patch

[Brian Anderson]

Created attachment 150699 [details]
Patch

[Brian Anderson]

Comment on attachment 150699 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:135
> +        int partialUploadCount = 0;

I didn't use index here, because there were a lot of ugly off-by-one adjustments needed without a separate partialUploadCount variable.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:51
> +// Tracks method call patterns across different classes.

There are no more dependencies on gmock, but I stuck with a GlobalTracker and very simple FakeWebGraphicsContext3D and FakeTextureUploader classes that forward their calls to the global tracker.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:108
> +    void verify()

I know we wanted to put these checks in the tests themselves, but the code became repetitive and long so I put the checks in a verify() method.

[Nat Duca]

Comment on attachment 150699 [details]
Patch

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

Sorry, the test isn't looking quite right, yet. You've got to do something about this global tracker, which is a non-normative design pattern. I think you can just make each of the MyXXX classess have pointers to the CCTextureUpdaterTest object and move all the GlobalTracker fields to the Test body.

You also still need to make the tests "say what they're testing." Right now, they set up different situations, which is good. But they are just saying "verify" but its not clear what they're verifying. Maybe see what the verification code looks like when you hand-type-it-out each time? Many of the test cases dont need every single item in your verify() method to be done, too. You only need to verify things that are interestnig.

I do see that you have a lot of *very* similar cases for these remainder tests [which I dont quite grok]. For those, you could make a subclass of CCTextureUpdaterTest, eg. CCTextureUpdaterRemainderTest that has the verify function it. That would be maybe be okay.

As I note below, I'm a bit worried that all the tests pass kMax to the updater. The common use case is "lots of updates, and kMax=40 [or 12]". Do we have good coverage of that case?

Any idea why its not cleanly applying to ToT? Those purple indicators on the patch usually imply patch failure.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:107
> +        int uploadCount = 0;

fullUploadCount?

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:135
>> +        int partialUploadCount = 0;
> 
> I didn't use index here, because there were a lot of ugly off-by-one adjustments needed without a separate partialUploadCount variable.

Erm, just for my education, when does (index + 1) % kUploaderFlushPeriod go wrong?

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:142
> +class MyFakeWebGraphicsContext3D : public FakeWebGraphicsContext3D {

Why not construct this with a backpointer to CCTextureUpdaterTest? Then this "GlobalTracker" can be the CCTextureUpdaterTest? I don't see value add in this GlobalTracker object.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:236
> +    m_globalTracker.verify();

Just verify inline. This one doesn't need to verify all the pieces... here, you just need to make sure no flushes were done.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:241
> +TEST_F(CCTextureUpdaterTest, OneTextureUploadA)

OneFullTextureUpload

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:246
> +    m_globalTracker.verify();

Here, you want really to verify that what, one flush was done? I dont think you need any other verifications here.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:249
> +TEST_F(CCTextureUpdaterTest, OneTextureUploadB)

OnePartialTexturUPload

xxxA and xxxB is not informative to a person not familiar with this code desperately trying to understand why a test is failing.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:257
> +TEST_F(CCTextureUpdaterTest, OneTextureUploadC)

OnePartialOneFull

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:262
> +    m_globalTracker.verify();

Again, I think the number of things you really care about here is that we did one flush, right? Or two? Or none?

By way of example, in this case, I know your code pretty well but I can't even quite remember how many flushes we do here. And, to figure out what the correct expectation is, I've actually got to study the code in detail. Versus just reading a number.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:266
> +// NO REMAINDER TESTS

You should explain what this family of tests is doing. I'm a bit puzzled what you're trying to test here, I suspect it has to do with some quirk with % logic, but its not obvious to me from inspection.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:274
> +    m_updater.update(m_context.get(), &m_allocator, &m_copier, &m_uploader, kMaxUploads);

Why are you passing kMaxUploads to all of these test methods? It seems to me that somewhere, you want to pass like something smaller [like the value we pass into the updater from CCThreadProxy?]

[Brian Anderson]

Comment on attachment 150699 [details]
Patch

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

patch set incoming that should address the comments.  will likely need some more feedback however

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:107
>> +        int uploadCount = 0;
> 
> fullUploadCount?

fixed

>>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:135
>>> +        int partialUploadCount = 0;
>> 
>> I didn't use index here, because there were a lot of ugly off-by-one adjustments needed without a separate partialUploadCount variable.
> 
> Erm, just for my education, when does (index + 1) % kUploaderFlushPeriod go wrong?

"(index + 1) % kUploaderFlushPeriod" is correct, but then line 145 needs to be "if(index % kUploadFlushPeriod)", which looks inconsistent and confusing.

Another option would be to move "++index" to where "partialUploadCount++" is, but that breaks the normal for loop flow.

In the end, I kept partialUploadCount to be consistent with fullUploadCount, but I would be fine with either of the two other options.

[Brian Anderson]

Created attachment 151847 [details]
Patch

[Nat Duca]

Comment on attachment 151847 [details]
Patch

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

This looks good Brian. Thank you for putting up with all the iterations. @enne, over to you for review.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:52
> +public:

You can just predeclare CCTextureUpdaterTest. Then instead of separating the decl of the notifier, put the implementation of MyFakeWGC3D::flush and anything else that dereferneces the class at the bottom of the file.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:60
> +class MyFakeWebGraphicsContext3D : public FakeWebGraphicsContext3D {

WebGraphicsContext3DForUploadTest.

I think this is a stub. I know I told you fake, but I can never remember. I had to go read this again. :) And, note, we use Fake wrong in our code, because I'm constantly confused. :/

http://martinfowler.com/articles/mocksArentStubs.html

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:63
> +    virtual void flush(void) { m_test->onFlush(); }

implement this method at the bottom of the file

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:71
> +class MyFakeTextureUploader : public FakeTextureUploader {

Shouldn't be called MyFakeTextureUploader. I gave you that, I realize, in pseudocode, but I was brain fragged. :/ You want something like TextureUploaderForUploadTest, maybe

Also a stub.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:76
> +    virtual void endUploads() { m_test->onEndUploads(); }

and implement these methods at the bottom of the file

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:396
> +// multiple calls to update()

End sentence with period.

[Adrienne Walker]

Comment on attachment 151847 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add flushes to CCTextureUpdater::update

This is a Chromium-only bug, so please prefix the bug title with [chromium] so that other ports don't have to pay any attention to this patch.

> Source/WebCore/ChangeLog:10
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> +
> +        No new tests (OOPS!).

You need to fill this out and remove the OOPS lines.

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

You need to fill this out too.

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:52
> +    void flush() { m_context3D->flush(); }
> +

m_context3D could be null.  I think you need to if-check it here.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:167
> +            else if (m_numTotalUploads > m_fullUploadCountExpected
> +                     && m_numTotalUploads < m_totalUploadCountExpected)
> +                EXPECT_GE(m_numDanglingUploads, kFlushPeriodPartial) << "Premature flush detected in partial uploads.";

style nit: WebKit line lengths are not that restrictive.  Put the whole condition on one line for clarity.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:203
> +            EXPECT_EQ(m_maxUploadCountPerUpdate, m_numPreviousUploads)
> +                    << "endUpload() was called when there are textures to upload, but the upload quota hasn't been filled.";

style nit: Just indent with four spaces when a line is too long.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:497
> +    // Note, this is testing an implementation detail that, if the partial
> +    // uploads would make us go over the max-uploads-per-update, then just
> +    // defer all the partial updates. It would, however, also be valid to see
> +    // 10 partial uploads in addition to the 30 full uploads.
> +    expectedPreviousUploads = kFullUploads-kMaxUploadsPerUpdate;
> +    EXPECT_EQ(expectedPreviousUploads, m_numPreviousUploads);
> +    EXPECT_NE(expectedPreviousUploads, kMaxUploadsPerUpdate) << "You may actualy be okay here; consider changing the fragile test.";

Maybe just don't test this implementation detail, then? General upload behavior is tested elsewhere.

s/actualy/actually/

[Brian Anderson]

Created attachment 152368 [details]
Patch

[Brian Anderson]

Comment on attachment 152368 [details]
Patch

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

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:458
> +    EXPECT_EQ(3,  m_numBeginUploads);

Need to remove a space after the comma.

[Brian Anderson]

Created attachment 152370 [details]
Patch

[Adrienne Walker]

Comment on attachment 152370 [details]
Patch

R=me.  Looks great!

[WebKit Review Bot]

Comment on attachment 152370 [details]
Patch

Clearing flags on attachment: 152370

Committed r122654: <http://trac.webkit.org/changeset/122654>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.