Bug 81004 – [Chromium] Use GL_CHROMIUM_command_buffer_query to throttle texture uploads.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Estimate incremental texture update time based on results from CHROMIUM_fence based occlusion queries.

[David Reveman]

Created attachment 131634 [details]
Patch

[David Reveman]

Created attachment 131760 [details]
Patch

[David Reveman]

Created attachment 133436 [details]
Patch

[David Reveman]

Created attachment 133607 [details]
Patch

[Nat Duca]

Comment on attachment 133607 [details]
Patch

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

Awesome david. Key suggestion below is that we set some constants for our the cost-per-upload so we can get this v0 patch ready for landing... that sound good? I really like where this is going! Dont even worry about proving that this is a perf improvement... just focus on making it committable and we'll deal with perf after the fact. I know its going in the right direction. :)

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:49
> +static const size_t textureUpdatesPerFrame = 4;

Per frame?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:58
> +const double displayRefreshInterval = 1.0 / 60.0;

This isn't fixed... only the scheduler should know this...

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:529
> +    m_nextTickTime = nextTickTime;

Can we fold the actual logic into the m_currentTextureUpdater object? E.g.

m_currentTextureUpdater->beginUpdatingUntil(nextTickTime)

And it would own the m_textureUpdateTimer [we'd initialize the updater with a pointer to the impl thread, etc]

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:761
> +class CCTextureUpdateQuery {

I think you might call this something like CCQuery and pass in the query type. And then move it to its own header. And write a unit test for it against a fake gc3d.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:849
> +double CCThreadProxy::currentTextureUpdateTime()

confused by this. i almost think you should have the processTextureUpdateQueries push results into a completedValues vector where maybe the completedvalues is {frameNumber, timestamp} and you use only the last 10 frames or something and then purge. I think its better to separate out the query logic from the measurement/data tracking, basically.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:869
> +double CCThreadProxy::processTextureUpdateQueries()

processCompletedQueries()

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:872
> +        if (!m_pendingQueriesList.last()->isAvailable())

an't you do if (!last) return

while(!pendingQueriesList.isEmpty) {
assert(isavailable) && process // but no available check?
}

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:879
> +    return currentTextureUpdateTime();

not sure why you're returning a value here. Beter to separate concerns, as I mentioned above.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:886
> +void CCThreadProxy::adjustTextureUpdatesPerFrame(double textureUpdateTime)

I'm concerned that we'll get distracted by control system issues and not get the basic fence system ready to land. You point out correctly that this is complex. :)

So, proposal: v0 of this patch should just hard code a value "perTextureUploadCostInMS." I suggest we set it to be 0.75 ms.

We can then do a followon patch **only when we've done the v0*** that tries to compute that value dynamically. SG?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:904
> +    if (maxTextureUpdateTime > displayRefreshInterval * 0.75) {

assume that the sceduler told us to update at "lastDoMoreUpdatesTime" and told us nextFrameTime.

can this be 3/4ths of nextFramTime - lastDoMoreUPdatesTime?

And, at that poitn, should the scheduler just do this logic itself and instead of passing nextFrameTime, pass instead the stopUpdatingAtThisTime?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:918
> +    size_t updates = min(m_currentTextureUpdaterOnImplThread->numUpdates(), textureUpdatesPerFrame);

is this textureUpdatesPerFrame, or textureUpdatesPerTick?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:923
> +    if (updates == textureUpdatesPerFrame) {

what's this check doing?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:927
> +        } else if (m_pendingQueriesList.size() < maxPendingTextureUpdateQueries)

I think we can streamline this a bit...


start(nextFrameTime) {
  m_nextFrametime = 0.75 * (m_nextFrameTime - now) + now
  m_timer.starT();

tick() {
  prcoessTextureUpdateQueries()
  if (too much pending) return
  if (too close to next frame time) return
  do a bit more uploads
  issue a fence

What am I missing?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:933
> +            CCProxy::implThread()->postDelayedTask(createCCThreadTask(this, &CCThreadProxy::textureUpdateOnImplThread), textureUpdateRetryIntervalMs);

See CCTimer

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:941
> +    m_currentTextureUpdaterOnImplThread->update(m_layerTreeHostImpl->context(), textureUpdatesPerFrame);

once we move this code into the updater, things get even cleaner, right?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:954
> +        m_textureUpdateTimerOnImplThread->startOneShot(textureUpdateTime * updates);

Lets just set this liberally --- e.g. tick every 4ms or something...

[David Reveman]

(In reply to comment #5)
> (From update of attachment 133607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133607&action=review
> 
> Awesome david. Key suggestion below is that we set some constants for our the cost-per-upload so we can get this v0 patch ready for landing... that sound good? I really like where this is going! Dont even worry about proving that this is a perf improvement... just focus on making it committable and we'll deal with perf after the fact. I know its going in the right direction. :)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:49
> > +static const size_t textureUpdatesPerFrame = 4;
> 
> Per frame?

I kept the existing name of that constant but that should of course change now that we're no longer waiting until the next frame to do more updates. How about PerBatch or PerTick instead?

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:58
> > +const double displayRefreshInterval = 1.0 / 60.0;
> 
> This isn't fixed... only the scheduler should know this...

Makes sense. I'll get it from the scheduler instead of using the same value as the scheduler is initialized with.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:529
> > +    m_nextTickTime = nextTickTime;
> 
> Can we fold the actual logic into the m_currentTextureUpdater object? E.g.
> 
> m_currentTextureUpdater->beginUpdatingUntil(nextTickTime)
> 
> And it would own the m_textureUpdateTimer [we'd initialize the updater with a pointer to the impl thread, etc]

With the current lifetime of the texture updater, the queries and the query results have to live somewhere else. Should we try to change the lifetime of the texture updater and have it be the owner of the queries or create an interface that allows them to live elsewhere?

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:761
> > +class CCTextureUpdateQuery {
> 
> I think you might call this something like CCQuery and pass in the query type. And then move it to its own header. And write a unit test for it against a fake gc3d.

Good idea.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:849
> > +double CCThreadProxy::currentTextureUpdateTime()
> 
> confused by this. i almost think you should have the processTextureUpdateQueries push results into a completedValues vector where maybe the completedvalues is {frameNumber, timestamp} and you use only the last 10 frames or something and then purge. I think its better to separate out the query logic from the measurement/data tracking, basically.

I just put the simplest thing I could think of in this patch. A separate vector with results from completed queries would be a possible improvement. I've also experimented with extrapolating the texture update time from previous measurements and only allowing a certain rate of change. That provided nice stable values for the texture update time.

How about we keep this logic in a separate object. Maybe a CCTextureUpdateMetrics?

class CCTextureUpdateMetrics {
public:                                                                         
    virtual void addResult(double result, double timestamp) = 0;
    virtual double estimatedUpdateTime() = 0;
};

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:869
> > +double CCThreadProxy::processTextureUpdateQueries()
> 
> processCompletedQueries()

ok.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:872
> > +        if (!m_pendingQueriesList.last()->isAvailable())
> 
> an't you do if (!last) return
> 
> while(!pendingQueriesList.isEmpty) {
> assert(isavailable) && process // but no available check?
> }
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:879
> > +    return currentTextureUpdateTime();
> 
> not sure why you're returning a value here. Beter to separate concerns, as I mentioned above.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:886
> > +void CCThreadProxy::adjustTextureUpdatesPerFrame(double textureUpdateTime)
> 
> I'm concerned that we'll get distracted by control system issues and not get the basic fence system ready to land. You point out correctly that this is complex. :)

I added this as I thought it be useful information when tracing. It could have been named "textureUpdatePerformanceIsTooLowForFrameRate".

> 
> So, proposal: v0 of this patch should just hard code a value "perTextureUploadCostInMS." I suggest we set it to be 0.75 ms.
> 
> We can then do a followon patch **only when we've done the v0*** that tries to compute that value dynamically. SG?

so the v0 would ignore the query results then, right? sure, I don't mind doing that first.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:904
> > +    if (maxTextureUpdateTime > displayRefreshInterval * 0.75) {
> 
> assume that the sceduler told us to update at "lastDoMoreUpdatesTime" and told us nextFrameTime.
> 
> can this be 3/4ths of nextFramTime - lastDoMoreUPdatesTime?
> 
> And, at that poitn, should the scheduler just do this logic itself and instead of passing nextFrameTime, pass instead the stopUpdatingAtThisTime?

we could move this logic into the scheduler but it would then have to know about the texture update time and deal with the problem of maxTextureUpdateTime being larger than the refresh interval. I think we can ignore this for v0 where we use a hard-coded updated time.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:918
> > +    size_t updates = min(m_currentTextureUpdaterOnImplThread->numUpdates(), textureUpdatesPerFrame);
> 
> is this textureUpdatesPerFrame, or textureUpdatesPerTick?

PerTick.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:923
> > +    if (updates == textureUpdatesPerFrame) {
> 
> what's this check doing?

it's to avoid using queries for updates that include partial uploads.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:927
> > +        } else if (m_pendingQueriesList.size() < maxPendingTextureUpdateQueries)
> 
> I think we can streamline this a bit...
> 
> 
> start(nextFrameTime) {
>   m_nextFrametime = 0.75 * (m_nextFrameTime - now) + now
>   m_timer.starT();
> 
> tick() {
>   prcoessTextureUpdateQueries()
>   if (too much pending) return
>   if (too close to next frame time) return
>   do a bit more uploads
>   issue a fence
> 
> What am I missing?

This is pretty much what the current patch is doing except for using a separate timers for "too much pending" and "do a bit more uploads". I don't think we want it to wait for the next update tick every time there's too many pending updates. Also my current patch takes partial texture updates into account and basically considering them free.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:933
> > +            CCProxy::implThread()->postDelayedTask(createCCThreadTask(this, &CCThreadProxy::textureUpdateOnImplThread), textureUpdateRetryIntervalMs);
> 
> See CCTimer
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:941
> > +    m_currentTextureUpdaterOnImplThread->update(m_layerTreeHostImpl->context(), textureUpdatesPerFrame);
> 
> once we move this code into the updater, things get even cleaner, right?

I think that would clean some of this up, yes.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:954
> > +        m_textureUpdateTimerOnImplThread->startOneShot(textureUpdateTime * updates);
> 
> Lets just set this liberally --- e.g. tick every 4ms or something...

Ok, I think I have a pretty good idea of what you want v0 to look like. I think most of the things mentioned above aren't relevant for v0. The result will be pretty different from my current patch that avoids setting a limit to the update performance but I'm fine with doing this as a follow up patch.

[Nat Duca]

(In reply to comment #6)
> I kept the existing name of that constant but that should of course change now that we're no longer waiting until the next frame to do more updates. How about PerBatch or PerTick instead?

Thats good. Per batch soudns good.

> With the current lifetime of the texture updater, the queries and the query results have to live somewhere else. Should we try to change the lifetime of the texture updater and have it be the owner of the queries or create an interface that allows them to live elsewhere?

Yeah, lets try that out. Maybe the updater should always exist and we just push more items into it as we get them?


> How about we keep this logic in a separate object. Maybe a CCTextureUpdateMetrics?
> 
> class CCTextureUpdateMetrics {
> public:                                                                         
>     virtual void addResult(double result, double timestamp) = 0;
>     virtual double estimatedUpdateTime() = 0;
> };

I really like this idea... its a scheduler! CCTextureUpdateScheduler? I think this makes it possible to write a v1 and even mock out the control logic from the streaming logic, even... I think my main hangup in your previous patch was that I couldn't see the scheduling logic separately from the basic query logic.

What if we make the interface be more like "I just uploaded this specific tile/texture, and the result was this" and "should I update this tile now?" That way we could unit test the TextureUpdateScheduler separately from the streaming logic in the basic TextureUpdater, and the interface is a little less specific about how the scheduling works.


> I added this as I thought it be useful information when tracing. It could have been named "textureUpdatePerformanceIsTooLowForFrameRate".
Heheh

> so the v0 would ignore the query results then, right? sure, I don't mind doing that first.
Yeah. We're still getting a win by getting better interleaving between proceses.


> > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:904
> > > +    if (maxTextureUpdateTime > displayRefreshInterval * 0.75) {
> we could move this logic into the scheduler but it would then have to know about the texture update time and deal with the problem of maxTextureUpdateTime being larger than the refresh interval. I think we can ignore this for v0 where we use a hard-coded updated time.

Good point. Okay. :)

> > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:923
> > > +    if (updates == textureUpdatesPerFrame) {
> > 
> > what's this check doing?
> 
> it's to avoid using queries for updates that include partial uploads.
Do they behave differently? I forget how this all fits together, I'm embarassed to say.


> This is pretty much what the current patch is doing except for using a separate timers for "too much pending" and "do a bit more uploads". I don't think we want it to wait for the next update tick every time there's too many pending updates. Also my current patch takes partial texture updates into account and basically considering them free.

Yeah, you're totally right. I'm sorry about that.

> Ok, I think I have a pretty good idea of what you want v0 to look like. I think most of the things mentioned above aren't relevant for v0. The result will be pretty different from my current patch that avoids setting a limit to the update performance but I'm fine with doing this as a follow up patch.


You know, with your metrics (controller) object, I think it we might be able to get your dynamic stuff in anyways. So, as long as we get the time-control bit cleanly separated from the query/streaming system, I'm happy.

[Nat Duca]

(In reply to comment #7)
> (In reply to comment #6)
Any update?

[David Reveman]

Created attachment 134732 [details]
Patch

Updated patch. Needs new unit tests.

[David Reveman]

Created attachment 134835 [details]
Patch

Add unit tests.

[Nat Duca]

Comment on attachment 134835 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:131
> +    m_stateMachine.setCanUpdateMoreResources(m_stateMachine.redrawPending() ? monotonicallyIncreasingTime() < (m_frameRateController->nextTickTime() - m_client->maxResourceUpdateTime()) : true);

hard to read. make cleaner?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:241
> +    if (resourcesPending)

Why are you merging state about beginFrameComplete with resources pending?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:799
> +void CCThreadProxy::deleteTextureUpdateQueries()

Wait, I thought you were going to move most of this into a standalone class? I'm going to hold additional feedback until I understand where we stand on that bit of refactoring.

[David Reveman]

(In reply to comment #11)
> (From update of attachment 134835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134835&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:131
> > +    m_stateMachine.setCanUpdateMoreResources(m_stateMachine.redrawPending() ? monotonicallyIncreasingTime() < (m_frameRateController->nextTickTime() - m_client->maxResourceUpdateTime()) : true);
> 
> hard to read. make cleaner?

sure.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:241
> > +    if (resourcesPending)
> 
> Why are you merging state about beginFrameComplete with resources pending?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:799
> > +void CCThreadProxy::deleteTextureUpdateQueries()
> 
> Wait, I thought you were going to move most of this into a standalone class? I'm going to hold additional feedback until I understand where we stand on that bit of refactoring.

Yes, I started doing that for this patch but realized that it wasn't obvious what that class would look like.

Creating a CCQuery class with a constructor that takes a GraphicsContext3D pointer is not necessarily what we want. All existing wrappers around GL objects that contain a pointer to a GC3D live in LayerRendererChromium. The LRC constructor takes a GC3D so the validity of the GC3D pointers passed to these wrappers is straight forward. How about GL object wrappers that don't live in the LRC?
Maybe all these CCQuery objects should live in the LRC? But I'm not sure that's appropriate and if they don't live there what should the destruction of these instances look like?

Maybe we should use the same Manager/Allocator system that we use for texture objects? It seems overkill to use that for objects that are only used on the impl thread. But on the other hand use on both threads might be something we need. I'll most likely need it for my shared memory stuff.

What do you think?

[Nat Duca]

We (In reply to comment #12)
> (In reply to comment #11)
Good point.

How about we fold this into the CCTextureUpdater class, and make the texture updater constructed with a GC3D pointer [raw pointer].

Then when the context is lost, (CCThreadProxy::didLose) we need to delete the current texture updater. Or something ~= that....

[David Reveman]

Created attachment 135103 [details]
Patch

Move some throttling code into CCTextureUpdater.

[Dana Jansens]

Comment on attachment 135103 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:134
> +    double canUpdateMoreResources = false;
> +    if (m_stateMachine.redrawPending())
> +        canUpdateMoreResources = true;

double = false/true?

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:140
> +    m_stateMachine.setCanUpdateMoreResources(canUpdateMoreResources);

Would it be better to compute all of canUpdateMoreResources with assignments and boolean logic instead of branching twice with if()?

like..
bool hasRedrawPending = ...
bool hasTimeRemaining = ...
bool canUpdateMoreResources = hasRedrawPending || hasTimeRemaining;

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:77
> +    if (m_needsForcedRedraw)
> +        return true;

shouldDraw() is always true in this case, so COMMIT_STATE_UPDATING_RESOURCES would always return ACTION_DRAW*, and never call this function with this value, right?

If this is meant to cause updating resources, maybe that method should be reordered or something? Can there be a unit test for each of the cases in this method?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:80
> +    if (m_updateMoreResourcesPending)
> +        return false;

Random question: why does moreResourcesPending mean we don't want to update? And this seems to be opposite of what was deleted from COMMIT_STATE_UPDATING_RESOURCES?

[David Reveman]

Created attachment 135169 [details]
Patch

Add CCTextureUpdater::Query class, fix canUpdateMoreResources computation in CCScheduler::nextAction, remove pendingResource argument from CCSchedulerStateMachine::beginFrameComplete and remove unnecessary context argument from updateCompositorResources.

[David Reveman]

Sorry about that last patch. Not what I intended to upload. 

(In reply to comment #15)
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:140
> > +    m_stateMachine.setCanUpdateMoreResources(canUpdateMoreResources);
> 
> Would it be better to compute all of canUpdateMoreResources with assignments and boolean logic instead of branching twice with if()?
> 
> like..
> bool hasRedrawPending = ...
> bool hasTimeRemaining = ...
> bool canUpdateMoreResources = hasRedrawPending || hasTimeRemaining;

Good idea, I included this in the latest patch. FYI, previous patch had hasRedrawPending wrong. We only care about if we have enough time when there is a redraw pending.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:77
> > +    if (m_needsForcedRedraw)
> > +        return true;
> 
> shouldDraw() is always true in this case, so COMMIT_STATE_UPDATING_RESOURCES would always return ACTION_DRAW*, and never call this function with this value, right?

True.

> 
> If this is meant to cause updating resources, maybe that method should be reordered or something? Can there be a unit test for each of the cases in this method?

I think that check should just be removed and forced redraw shouldn't really affect resource update scheduling.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:80
> > +    if (m_updateMoreResourcesPending)
> > +        return false;
> 
> Random question: why does moreResourcesPending mean we don't want to update? And this seems to be opposite of what was deleted from COMMIT_STATE_UPDATING_RESOURCES?

m_updateMoreResourcesPending indicate that there's resource update pending. We don't want to start another update while there's one already pending.

[David Reveman]

Created attachment 135176 [details]
Patch

Remove m_needsForcedRedraw check from CCSchedulerStateMachine::shouldUpdateMoreResources

[Nat Duca]

Comment on attachment 135176 [details]
Patch

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

Getting late here, I got to the CCThreadProxy in the first pass, will post comments so you can review them, will try to do more reviewing tomorrow.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:125
> +    // Indicates whether updating resources would, at this time, make sense.

Help me understand what this state is for? It looked like you had gotten everything to fit into the "we are updating resources" state.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:136
> +void CCTextureUpdater::update(size_t count)

I guess we need to pass in the "count" variable to support singlethreadproxy.

Is there any way to factor this so the count constant is inside this file?

What about
updateUntilComplete(), updateUntilDeadlineExpired(), with update(count) being private?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:138
> +    OwnPtr<Query> query;

can we factor this into a few protected functions that are relatively obviously named? Look at CCLayerAnimationController for an example of what I was sort of thinking. It really helps make the steps more obvious.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:140
> +    if (m_updateTimer) {

So maybe we should just have updateEverything and updateStreaming... and not actually try to share code between the two paths. I think we're losing clarity in this code by trying to make it support both the CCSingleThreadProxy use case and the CCThreadProxy use case. It seems to me we that the amount of code the updateEverything vs streaming code duplicates is pretty minimal.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:147
> +        if (!m_availableQueriesList.isEmpty()) {

This block is still really hard to follow.

Can't this be factored more like "if num pending queries, then return". Then, once you've done that discard, grab a new query, pulling from a freelist if needed.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:186
> +        m_pendingQueriesList.prepend(query.release());

prepend? I guess you're doing fifo but with the "front" being the back... eep!

Why not just make pendingQueriesList be a deque? Wtf has such a thing, iirw.

[David Reveman]

(In reply to comment #19)
> (From update of attachment 135176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135176&action=review
> 
> Getting late here, I got to the CCThreadProxy in the first pass, will post comments so you can review them, will try to do more reviewing tomorrow.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:125
> > +    // Indicates whether updating resources would, at this time, make sense.
> 
> Help me understand what this state is for? It looked like you had gotten everything to fit into the "we are updating resources" state.

The "canUpdateMoreResources" flag gives the scheduler the ability to tell the state machine that now is not a good time to begin updating more resources event though we're in the updating-resources state and there are updates pending. The scheduler will set this to false when there's a redraw pending and we're close to the vsync tick time.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:136
> > +void CCTextureUpdater::update(size_t count)
> 
> I guess we need to pass in the "count" variable to support singlethreadproxy.
> 
> Is there any way to factor this so the count constant is inside this file?
> 
> What about
> updateUntilComplete(), updateUntilDeadlineExpired(), with update(count) being private?

It would be relatively easy to move the constant into CCTextureUpdater.cc but we can't make it completely private as the LTH needs to know what this is to support partial texture updates. If it doesn't know how many updates are done per tick it can't allow partial texture updates without the risk of commits not being atomic.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:138
> > +    OwnPtr<Query> query;
> 
> can we factor this into a few protected functions that are relatively obviously named? Look at CCLayerAnimationController for an example of what I was sort of thinking. It really helps make the steps more obvious.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:140
> > +    if (m_updateTimer) {
> 
> So maybe we should just have updateEverything and updateStreaming... and not actually try to share code between the two paths. I think we're losing clarity in this code by trying to make it support both the CCSingleThreadProxy use case and the CCThreadProxy use case. It seems to me we that the amount of code the updateEverything vs streaming code duplicates is pretty minimal.

I think that makes a lot of sense.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:147
> > +        if (!m_availableQueriesList.isEmpty()) {
> 
> This block is still really hard to follow.
> 
> Can't this be factored more like "if num pending queries, then return". Then, once you've done that discard, grab a new query, pulling from a freelist if needed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:186
> > +        m_pendingQueriesList.prepend(query.release());
> 
> prepend? I guess you're doing fifo but with the "front" being the back... eep!
> 
> Why not just make pendingQueriesList be a deque? Wtf has such a thing, iirw.

Ok, I'll have a look at that asap.

[David Reveman]

Created attachment 135512 [details]
Patch

[Nat Duca]

Comment on attachment 135512 [details]
Patch

Hmm this patch is getting epic. I'm able to review it, but barely, and I think james/Enne will probably have a harder time.

Is there a way to split the refactoring changes from the key updater changes? E.g. give the updater a graphics context as one patch, maybe?

Similarliy, I wonder, can we do the scheduler changes without actually introducing the new updating code? E..g here's a new way to call the updater, but when we call it, it'll just update 4 at a time.

Then we can have a patch that just does the streaming uploads...

Have the stability issues you were seeing with occlusion query been resolved, btw?

[David Reveman]

Created attachment 137767 [details]
Patch

[David Reveman]

Limiting this bug to just adding the new ThrottledTextureUploader class. Moving the rest of the changes into a new bug.

[Nat Duca]

Comment on attachment 137767 [details]
Patch

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

LGTM. Enne for real review, I think.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1416
> +        m_textureUploader = NonBlockingTextureUploader::create(m_context.get());

NonBlocking sounds like it never blocks.... it does, though, doesn't it?

[David Reveman]

(In reply to comment #25)
> (From update of attachment 137767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137767&action=review
> 
> LGTM. Enne for real review, I think.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1416
> > +        m_textureUploader = NonBlockingTextureUploader::create(m_context.get());
> 
> NonBlocking sounds like it never blocks.... it does, though, doesn't it?

It doesn't, at least from a compositor API perspective. I'm not very happy with that name though. UnthrottledTextureUploader might be better.

[Adrienne Walker]

Comment on attachment 137767 [details]
Patch

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

Thanks for splitting this patch up.  This was quite large enough to try to digest on its own.  Thanks also for all the thorough review iterations, Nat.  :)

This looks great in general.  I just have some minor style quibbles and questions.

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1416
>>> +        m_textureUploader = NonBlockingTextureUploader::create(m_context.get());
>> 
>> NonBlocking sounds like it never blocks.... it does, though, doesn't it?
> 
> It doesn't, at least from a compositor API perspective. I'm not very happy with that name though. UnthrottledTextureUploader might be better.

I like unthrottled.

Also, just from a testability and separation of concerns standpoint, I think the proxy should create the texture updater that it wants itself and just return a TextureUploader to LRC, since the proxy is the one that's responsible for using it properly.

> Source/WebCore/platform/graphics/chromium/TextureUploader.h:42
> +class NonBlockingTextureUploader : public TextureUploader {

Please put this in a separate file if you're going to put the class declaration in a header file.  Alternatively, since this is all pretty simple, you could hide both the declaration and definition away in CCSingleThreadProxy.cpp.

> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:86
> +    if (maxPendingUploads)
> +        m_maxPendingQueries = maxPendingUploads;

The fact that !maxPendingUploads implies use the default number of pending uploads is really opaque.  Can you make a separate setMaxPendingQueries function or explicitly have two different constructors?  Or, even just better documentation of what this value means would help.  Also, maxPendingUploads is a confusing name, since it's dealing with queries and not uploads.

> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.h:37
> +    class Query {

Can this inner class be protected and not public?

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:205
> +        while (updater.hasMoreUpdates())
> +            updater.update(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator(), m_layerTreeHostImpl->layerRenderer()->textureCopier(), m_layerTreeHostImpl->layerRenderer()->textureUploader(), 1);

How does this work in the single-threaded case where max partial texture updates are huge? Wouldn't you infinitely loop or hit that assert that total partial updates are smaller than count (which is 1, in this case)?  Maybe 1 should be maxPartialTextureUpdates()?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:101
> +            return true;

The boolean return value here seems redundant with hasMoreUpdates().  Can you simplify and pick one way or the other?

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:115
> +        // Make sure the number of updates including partial updates are not more
> +        // than |count|.

This is comment is kind of misleading.  I think you mean "We need another update batch if the number of updates remaining in |count| is greater than the remaining partial entries."  Also, is there an off-by-one issue here? Should that be <= instead of <? Do you think it'd be useful to write a unit test around batch quantities and partial updates?

[David Reveman]

Created attachment 139314 [details]
Patch

[David Reveman]

(In reply to comment #27)
> (From update of attachment 137767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137767&action=review
> 
> Thanks for splitting this patch up.  This was quite large enough to try to digest on its own.  Thanks also for all the thorough review iterations, Nat.  :)
> 
> This looks great in general.  I just have some minor style quibbles and questions.
> 
> >>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1416
> >>> +        m_textureUploader = NonBlockingTextureUploader::create(m_context.get());
> >> 
> >> NonBlocking sounds like it never blocks.... it does, though, doesn't it?
> > 
> > It doesn't, at least from a compositor API perspective. I'm not very happy with that name though. UnthrottledTextureUploader might be better.
> 
> I like unthrottled.

done.

> 
> Also, just from a testability and separation of concerns standpoint, I think the proxy should create the texture updater that it wants itself and just return a TextureUploader to LRC, since the proxy is the one that's responsible for using it properly.

agree, done.

> 
> > Source/WebCore/platform/graphics/chromium/TextureUploader.h:42
> > +class NonBlockingTextureUploader : public TextureUploader {
> 
> Please put this in a separate file if you're going to put the class declaration in a header file.  Alternatively, since this is all pretty simple, you could hide both the declaration and definition away in CCSingleThreadProxy.cpp.

now in CCSingleThreadProxy.cpp.

> 
> > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:86
> > +    if (maxPendingUploads)
> > +        m_maxPendingQueries = maxPendingUploads;
> 
> The fact that !maxPendingUploads implies use the default number of pending uploads is really opaque.  Can you make a separate setMaxPendingQueries function or explicitly have two different constructors?  Or, even just better documentation of what this value means would help.  Also, maxPendingUploads is a confusing name, since it's dealing with queries and not uploads.

Now two different constructors. I prefer uploads. That queries are used is an implementation detail.

> 
> > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.h:37
> > +    class Query {
> 
> Can this inner class be protected and not public?

It's already private.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:205
> > +        while (updater.hasMoreUpdates())
> > +            updater.update(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator(), m_layerTreeHostImpl->layerRenderer()->textureCopier(), m_layerTreeHostImpl->layerRenderer()->textureUploader(), 1);
> 
> How does this work in the single-threaded case where max partial texture updates are huge? Wouldn't you infinitely loop or hit that assert that total partial updates are smaller than count (which is 1, in this case)?  Maybe 1 should be maxPartialTextureUpdates()?

It worked fine as LTH->bufferedUpdates() returns false when single threaded proxy is used. I still changed it to maxPartialTextureUpdates() as that makes more sense.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:101
> > +            return true;
> 
> The boolean return value here seems redundant with hasMoreUpdates().  Can you simplify and pick one way or the other?

I removed the boolean return value.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:115
> > +        // Make sure the number of updates including partial updates are not more
> > +        // than |count|.
> 
> This is comment is kind of misleading.  I think you mean "We need another update batch if the number of updates remaining in |count| is greater than the remaining partial entries."  Also, is there an off-by-one issue here? Should that be <= instead of <? Do you think it'd be useful to write a unit test around batch quantities and partial updates?

Updated the comment. "<" is correct there as we want that check to be false if count is equal to number of partial updates. Added a unit test to verify that 4 partial updates are done in one batch when batch size is 4.

[David Reveman]

Created attachment 140549 [details]
Patch

Rebase

[Adrienne Walker]

Comment on attachment 140549 [details]
Patch

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

R=me.  Thanks for all the changes.  I have a few more small nits, but nothing major.

What's the status on the bug with command buffer queries that gman was going to look into? Please make sure that is fixed before landing this.

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:103
> +class FakeTextureUploader : public TextureUploader {

Please consolidate all these FakeTextureUploader classes.  Maybe just use the common one?

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:978
> +    // Partail update of 4 tiles.

s/Partial/Partial/

[David Reveman]

Created attachment 140569 [details]
Patch

Remove duplicate implementations of FakeTextureUploader and don't enable ThrottledTextureUploader just yet

[Adrienne Walker]

Comment on attachment 140569 [details]
Patch

Landing infrastructure now and flipping the switch later sounds good.  R=me, again.

[David Reveman]

(In reply to comment #31)
> (From update of attachment 140549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140549&action=review
> 
> R=me.  Thanks for all the changes.  I have a few more small nits, but nothing major.
> 
> What's the status on the bug with command buffer queries that gman was going to look into? Please make sure that is fixed before landing this.

Not sure what the status of that bug is. I changed CCThreadProxy to not yet use a ThrottledTextureUploader so that we can land this patch and just enable the ThrottledTextureUploader once the queries bug has been fixed.

> 
> > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:103
> > +class FakeTextureUploader : public TextureUploader {
> 
> Please consolidate all these FakeTextureUploader classes.  Maybe just use the common one?

Now using the version defined in CCTiledLayerTestCommon.h everywhere.

> 
> > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:978
> > +    // Partail update of 4 tiles.
> 
> s/Partial/Partial/

Fixed.

[WebKit Review Bot]

Comment on attachment 140569 [details]
Patch

Clearing flags on attachment: 140569

Committed r116351: <http://trac.webkit.org/changeset/116351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.