Bug 84281 – [Chromium] Schedule texture uploads based on hard-coded timer and vsync.

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

[David Reveman]

Improve interaction between vsync and texture uploads by performing uploads in smaller batches and use a hard-coded timer to emulate upload completion. This greatly reduces the chance of the compositor missing a vsync due to being busy with texture uploads.

[David Reveman]

Created attachment 137777 [details]
Patch

[Nat Duca]

Comment on attachment 137777 [details]
Patch

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

This is looking good. A few high level comments.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:57
> +    virtual double maxResourceUpdateTime() const = 0;

expectedSecondsToPerformNextBlahBlah()?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:557
> +    return textureUpdateTickRate + 0.002;

maybe a // to explain rationale for this constant. I think I follow, but i'll forget a week from now. :)

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:825
> +    m_currentTextureUpdateTimer.clear();

You can avoid recreating the timer every time and just persist the timer and startOneShot every time.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:826
> +    m_schedulerOnImplThread->didUpdateMoreResourcesComplete();

I'm a little confused at the role of this timer in the throttled case... where do we tell the updater to check the fences? Or is the idea that this just causes the scheduler to re-hammer schedulActionUpdateMore and then that in-turn cauess a fence-check?

What if we move this timer into the ccscheduler [but not the state machine]? E.g. have scheduler set a timer that calls back to itself that then just hammers the allocator again...

[David Reveman]

Created attachment 140552 [details]
Patch

Rebased on current version of 81004.

[David Reveman]

(In reply to comment #2)
> (From update of attachment 137777 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137777&action=review
> 
> This is looking good. A few high level comments.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:57
> > +    virtual double maxResourceUpdateTime() const = 0;
> 
> expectedSecondsToPerformNextBlahBlah()?

Done. Now expectedSecondsToPerformNextUpdateMoreResources().

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:557
> > +    return textureUpdateTickRate + 0.002;
> 
> maybe a // to explain rationale for this constant. I think I follow, but i'll forget a week from now. :)

Moved this into CCScheduler::nextAction() and used variable names that hopefully makes it very clear what this is for.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:825
> > +    m_currentTextureUpdateTimer.clear();
> 
> You can avoid recreating the timer every time and just persist the timer and startOneShot every time.

done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:826
> > +    m_schedulerOnImplThread->didUpdateMoreResourcesComplete();
> 
> I'm a little confused at the role of this timer in the throttled case... where do we tell the updater to check the fences? Or is the idea that this just causes the scheduler to re-hammer schedulActionUpdateMore and then that in-turn cauess a fence-check?

Yes, all the current throttled texture uploader does is make sure we don't go beyond a certain limit of pending texture uploads. The texture uploader just wont perform any uploads when the limit is reached. It's currently up to the CCProxy to determine how frequently to hammer the uploader. CCThreadProxy currently uses textureUpdateTickRate=0.004.

> 
> What if we move this timer into the ccscheduler [but not the state machine]? E.g. have scheduler set a timer that calls back to itself that then just hammers the allocator again...

The textureUpdateTickRate constant belongs on the same place as the textureUpdatesPerTick constant. Maybe we should move both of these into ccscheduler but to avoid having this patch grow again I'd prefer to leave that for a follow up patch.

[David Reveman]

Created attachment 140600 [details]
Patch

Rebase

[James Robinson]

Comment on attachment 140600 [details]
Patch

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

Nat should look at the scheduler changes, but here are some comments to get you started.

Is this something we want to do for 20 or wait for post-20?  My gut is that we could probably use some time playing with this to shake out any unexpected issues before we try to get it on a release train.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:145
> +    const double secondsOfPadding = 0.002;

I'm not sure what this number is supposed to represent. Can haz comment?

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

this is a big change. can you take some traces of scrolling with 30-40 tiles updated/frame and verify that we don't get limited more than the GPU can handle?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:557
> +    static PassOwnPtr<CCThreadProxyTextureUpdateTimer> create(CCThreadProxy* proxy) { return adoptPtr(new CCThreadProxyTextureUpdateTimer(proxy)); }

probably worth expanding this out

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:559
> +    virtual void onTimerFired()

OVERRIDE please

[David Reveman]

(In reply to comment #6)
> (From update of attachment 140600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140600&action=review
> 
> Nat should look at the scheduler changes, but here are some comments to get you started.
> 
> Is this something we want to do for 20 or wait for post-20?  My gut is that we could probably use some time playing with this to shake out any unexpected issues before we try to get it on a release train.

I don't think it makes sense to land this unless the throttled texture uploader can be enabled, which depends on http://code.google.com/p/chromium/issues/detail?id=123414 getting fixed. 

I'd be more comfortable landing all this post 20 but if the query bug is quickly solved and Nat likes to see this in 20, I'm OK landing it and staying on top of any issues it might cause.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:145
> > +    const double secondsOfPadding = 0.002;
> 
> I'm not sure what this number is supposed to represent. Can haz comment?

This constant is just for adding some padding to make it less likely that we schedule texture uploads that will prevent us from drawing in a timely manner when the next vsync tick comes around. I'm removing the constant from this patch as with the current timing returned from expectedSecondsToPerformNextUpdateMoreResources() this isn't really useful. We probably want to add this back later but it wont make any difference right now.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:51
> > +static const size_t textureUpdatesPerTick = 4;
> 
> this is a big change. can you take some traces of scrolling with 30-40 tiles updated/frame and verify that we don't get limited more than the GPU can handle?

4 is a bit low. This shouldn't matter much once we start adjusting the tick rate based on the timing results returned by our queries but that's for another patch.

The updates per tick should be set to 11.52 to keep the current upload cost estimate which is: 2.88 uploads/ms

Based on my traces on the latest chromeos hardware, 16 seems closer to the optimal number of updates/tick there but I'll set it to 12 for now as I would prefer to keep a change to the upload cost estimate in a separate patch.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:557
> > +    static PassOwnPtr<CCThreadProxyTextureUpdateTimer> create(CCThreadProxy* proxy) { return adoptPtr(new CCThreadProxyTextureUpdateTimer(proxy)); }

done.

> 
> probably worth expanding this out
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:559
> > +    virtual void onTimerFired()
> 
> OVERRIDE please

done.

[David Reveman]

Created attachment 140633 [details]
Patch

Remove secondsOfPadding and  change textureUpdatesPerTick to 12

[Nat Duca]

Comment on attachment 140633 [details]
Patch

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

So this LGTM, but I think its a little awkward that "texturpeUpdateComplete" really means "tell the texture uploader to try doing a bit more." I'd be happier with better naming that doesn't imply that the actual uploads are done. But, </bikeshed>

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:151
> +    void textureUpdateCompleteOnImplThread();

I'm a little bugged by this, as noted by overall nit. Alkso, textureUpdate or textureUpload? Isn't this uploading? Lets make sure all the nomenclature is normalized.

[David Reveman]

(In reply to comment #9)
> (From update of attachment 140633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140633&action=review
> 
> So this LGTM, but I think its a little awkward that "texturpeUpdateComplete" really means "tell the texture uploader to try doing a bit more." I'd be happier with better naming that doesn't imply that the actual uploads are done. But, </bikeshed>

How about I follow up with a patch that does the following?

CCSchedulerClient::scheduledActionUpdateMoreResources() -> CCSchedulerClient::scheduledActionBeginUpdateMoreResources()

CCScheduler::didUpdateMoreResourcesComplete() -> CCScheduler::didEndUpdateMoreResources()

CCThreadProxy::textureUpdateCompleteOnImplThread() ->
CCThreadProxy::endTextureUpdatesOnImplThread()

I think that would better describe how the new system works:

1. We begin a texture update period.
2. The proxy, updater and uploader does whatever it can during this period.
3. and then calls didEndUpdateMoreResources().

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:151
> > +    void textureUpdateCompleteOnImplThread();
> 
> I'm a little bugged by this, as noted by overall nit. Alkso, textureUpdate or textureUpload? Isn't this uploading? Lets make sure all the nomenclature is normalized.

textureUpdate is correct here as the proxy only interacts with the texture updater. The texture updater will perform both texture uploads and texture copies.

[Nat Duca]

(In reply to comment #10)
> How about I follow up with a patch that does the following?
> 
> CCSchedulerClient::scheduledActionUpdateMoreResources() -> CCSchedulerClient::scheduledActionBeginUpdateMoreResources()
> 
> CCScheduler::didUpdateMoreResourcesComplete() -> CCScheduler::didEndUpdateMoreResources()
> 
> CCThreadProxy::textureUpdateCompleteOnImplThread() ->
> CCThreadProxy::endTextureUpdatesOnImplThread()
> 
> I think that would better describe how the new system works:
> 
> 1. We begin a texture update period.
> 2. The proxy, updater and uploader does whatever it can during this period.
> 3. and then calls didEndUpdateMoreResources().
> 

SGTM
 might work the word "try" in there...

And is it update or upload?

[Nat Duca]

(In reply to comment #11)
> And is it update or upload?

Oh, I see. This is really messing with my head.

[James Robinson]

Comment on attachment 140633 [details]
Patch

Clearing review flag, let's do https://bugs.webkit.org/show_bug.cgi?id=86898 first.  Having scheduler-type logic spread around multiple classes scares me a lot.

[Nat Duca]

Hey David, want to do a hangout to figure out how to move this patch forward? Its good stuff, I think we just have to make the scheduling logic more palatable. :)

[David Reveman]

(In reply to comment #14)
> Hey David, want to do a hangout to figure out how to move this patch forward? Its good stuff, I think we just have to make the scheduling logic more palatable. :)

Sorry I dropped the ball on this one. Last time I looked at this, moving the timer code to CCScheduler isn't all that straight forward. I'll need to add a CCResourceUpdateRateController class that uses a CCTimeSource so that we can properly unit test it. It will make this patch quite a bit larger but I think it's the right way to do it. Let me know if you have any other ideas.

[Nat Duca]

I'm not convinced we need to move this to the scheduler, and in fact, that sounds like a lot of complexity. Thats why I suggested a GVC. If I can re-convince myself of this suspicion, I can talk with JamesR and reconcile his concerns with my point of view. But, the first step is I think reviewing the problem space on a whiteboard.

[David Reveman]

Created attachment 147164 [details]
Patch

Make texture upload throttling transparent to CCScheduler

[David Reveman]

The latest attached patch includes some the initial changes to make texture upload throttling transparent to CCScheduler. It adds a timeLimit argument to scheduledActionUpdateMoreResources, which allows the client to perform texture uploads for a certain amount of time. This doesn't yet move the timer code into CCTextureUpdater (more about this below) which I'll work on next but would like some feedback on these initial changes first.

The challenge with moving the timing code into CCTextureUpdater is that the GC3D and TextureUploader/Copier are currently passed as arguments to CCTextureUpdater::update and it would be inappropriate to hold on to these pointers past the return of that call. My current plan is to pass these pointers to the CCTextureUpdater constructor instead along with the CCTimer that the updater will use. Does that sound reasonable?

[Nat Duca]

(In reply to comment #18)
> The challenge with moving the timing code into CCTextureUpdater is that the GC3D and TextureUploader/Copier are currently passed as arguments to CCTextureUpdater::update and it would be inappropriate to hold on to these pointers past the return of that call. My current plan is to pass these pointers to the CCTextureUpdater constructor instead along with the CCTimer that the updater will use. Does that sound reasonable?

So it will hold onto both gc3d and uploader pointers?

Have you thought about moving the entire updater to the lrc? Then just havign the proxies call to LRC as needed? I feel like something is wrong with our basic design here that is causing us unnecessary pain. If we could figure out a refactoring that reduces this complexity, it'd be great.

[Nat Duca]

Comment on attachment 147164 [details]
Patch

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

Not quite. We need to seal stuff inside the updater. If that means moving the updater to LRC, so be it.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:61
> +    virtual void scheduledActionUpdateMoreResources(double timeLimit) = 0;

timeLimitMonotonic

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:51
> +static const size_t textureUpdatesPerTick = 12;

This moves into the updater, right?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:547
> +class CCThreadProxyTextureUpdateTimer : public CCTimer, CCTimerClient {

Why is this on the proxy? You should be able to pass in an impl thread and have the updater construct a timer inside itself.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:628
> +    m_schedulerOnImplThread->setVisible(m_layerTreeHostImpl->visible());

why did this move?

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

this should be inside the updater!

[David Reveman]

(In reply to comment #20)
> (From update of attachment 147164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147164&action=review
> 
> Not quite. We need to seal stuff inside the updater. If that means moving the updater to LRC, so be it.

OK, maybe this was confusing but as I mentioned in my previous comment, I'm interested in some feedback to make sure the CCScheduler changes are good before I attempt to move the timer code into the texture updater. This patch is not complete. It's just a first step. Please ignore the fact that the timer code still lives in CCThreadProxy when looking at it.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:61
> > +    virtual void scheduledActionUpdateMoreResources(double timeLimit) = 0;
> 
> timeLimitMonotonic

Sure, I'll fix that.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:51
> > +static const size_t textureUpdatesPerTick = 12;
> 
> This moves into the updater, right?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:547
> > +class CCThreadProxyTextureUpdateTimer : public CCTimer, CCTimerClient {
> 
> Why is this on the proxy? You should be able to pass in an impl thread and have the updater construct a timer inside itself.

Yes, please ignore this for now. This is the next step.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:628
> > +    m_schedulerOnImplThread->setVisible(m_layerTreeHostImpl->visible());
> 
> why did this move?

This had to move as a result of the changes to CCScheduler. scheduledActionCommit() is now only called as a result of the vsyncTick, which means that the m_schedulerOnImplThread->setVisible() call will likely cause a draw to happen. Draw happening before m_layerTreeHostImpl->commitComplete() is called confuses our unit tests. They expect commitComplete() to be called before a draw happens, which I think is reasonably behavior.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:909
> > +void CCThreadProxy::textureUpdateOnImplThread()
> 
> this should be inside the updater!

Same as above, ignore for now.

If the CCScheduler changes look alright to you then I'll go ahead and start working on moving the timer code into the texture updater.

[Nat Duca]

Comment on attachment 147164 [details]
Patch

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

Oh, I get it. My bad, sorry for the grumpypants. :)

This looks goodish. Some thoughts below.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:167
> +                m_stateMachine.beginUpdateMoreResourcesComplete(false);

Can we delete this state on the state machine? I'm thinking we can make the state machine just track what frame number it last did a saUMR and not do more than one... this is sort of how we prevent draws too.

>>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:628
>>> +    m_schedulerOnImplThread->setVisible(m_layerTreeHostImpl->visible());
>> 
>> why did this move?
> 
> This had to move as a result of the changes to CCScheduler. scheduledActionCommit() is now only called as a result of the vsyncTick, which means that the m_schedulerOnImplThread->setVisible() call will likely cause a draw to happen. Draw happening before m_layerTreeHostImpl->commitComplete() is called confuses our unit tests. They expect commitComplete() to be called before a draw happens, which I think is reasonably behavior.

Maybe the real bug is that we draw inside scheduledActionCommit? I'm wondering if we should just have some code in ccscheduler that says "i'm in processscheduledactions" -- - calling psa inside psa will nop...

[David Reveman]

(In reply to comment #22)
> (From update of attachment 147164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147164&action=review
> 
> Oh, I get it. My bad, sorry for the grumpypants. :)
> 
> This looks goodish. Some thoughts below.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:167
> > +                m_stateMachine.beginUpdateMoreResourcesComplete(false);
> 
> Can we delete this state on the state machine? I'm thinking we can make the state machine just track what frame number it last did a saUMR and not do more than one... this is sort of how we prevent draws too.

That sounds reasonable but to keep this patch minimal and try to land it soon I'd prefer to avoid this change for now if possible?

> 
> >>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:628
> >>> +    m_schedulerOnImplThread->setVisible(m_layerTreeHostImpl->visible());
> >> 
> >> why did this move?
> > 
> > This had to move as a result of the changes to CCScheduler. scheduledActionCommit() is now only called as a result of the vsyncTick, which means that the m_schedulerOnImplThread->setVisible() call will likely cause a draw to happen. Draw happening before m_layerTreeHostImpl->commitComplete() is called confuses our unit tests. They expect commitComplete() to be called before a draw happens, which I think is reasonably behavior.
> 
> Maybe the real bug is that we draw inside scheduledActionCommit? I'm wondering if we should just have some code in ccscheduler that says "i'm in processscheduledactions" -- - calling psa inside psa will nop...

Yes, that's probably a good idea. Can we fix this separately?

[David Reveman]

Created attachment 147755 [details]
Patch

Add CCTextureUpdateController class and move all timing code into this class.

[Nat Duca]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 147164 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147164&action=review
> > 
> > Oh, I get it. My bad, sorry for the grumpypants. :)
> > 
> > This looks goodish. Some thoughts below.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:167
> > > +                m_stateMachine.beginUpdateMoreResourcesComplete(false);
> > 
> > Can we delete this state on the state machine? I'm thinking we can make the state machine just track what frame number it last did a saUMR and not do more than one... this is sort of how we prevent draws too.
> 
> That sounds reasonable but to keep this patch minimal and try to land it soon I'd prefer to avoid this change for now if possible?
> 
> > 
> > >>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:628
> > >>> +    m_schedulerOnImplThread->setVisible(m_layerTreeHostImpl->visible());
> > >> 
> > >> why did this move?
> > > 
> > > This had to move as a result of the changes to CCScheduler. scheduledActionCommit() is now only called as a result of the vsyncTick, which means that the m_schedulerOnImplThread->setVisible() call will likely cause a draw to happen. Draw happening before m_layerTreeHostImpl->commitComplete() is called confuses our unit tests. They expect commitComplete() to be called before a draw happens, which I think is reasonably behavior.
> > 
> > Maybe the real bug is that we draw inside scheduledActionCommit? I'm wondering if we should just have some code in ccscheduler that says "i'm in processscheduledactions" -- - calling psa inside psa will nop...
> 
> Yes, that's probably a good idea. Can we fix this separately?

I dont think I'm comfortable landing this for m21. The only pressure to land it is self-created, and while I understand you want to be done with it, I'd prefer we do this right. So, given that, can you think through how to work around these issues?

[Nat Duca]

Comment on attachment 147755 [details]
Patch

In particular, I think we should remove the updatespending state on the state machine and have the current updater be *owned* by LRC. Commit can set it directly and the proxy will just tick it. This fixes the ownership issues and allows us to have a considerably simper design I think.

[David Reveman]

(In reply to comment #26)
> (From update of attachment 147755 [details])
> In particular, I think we should remove the updatespending state on the state machine and have the current updater be *owned* by LRC. Commit can set it directly and the proxy will just tick it. This fixes the ownership issues and allows us to have a considerably simper design I think.

I'd like the updatesPending state change to land separately and I don't mind if that's done before we land this patch if that's what you prefer.

Making the current texture updater *owned* by LRC is not a trivial change. The texture updater is currently used for appending updates before the LRC is even created. So either we have to change LRC initialization significantly or change the texture updater.

The CCTextureUpdateController class in the current patch is making the necessary texture updater changes without changing the current CCTextureUpdater class. It would be relatively easy to move this new CCTextureUpdateController instance into the LRC.

If you prefer to change LRC initialization, then I'd like to see that done in a separate patch that we land before this one.

I'm comfortable landing the current patch as I'm confident it won't cause regressions unrelated to the feature it implements. This will change if we add the updatesPending state change and/or LRC initialization changes to it.

[James Robinson]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 147755 [details] [details])
> > In particular, I think we should remove the updatespending state on the state machine and have the current updater be *owned* by LRC. Commit can set it directly and the proxy will just tick it. This fixes the ownership issues and allows us to have a considerably simper design I think.
> 
> I'd like the updatesPending state change to land separately and I don't mind if that's done before we land this patch if that's what you prefer.
> 
> Making the current texture updater *owned* by LRC is not a trivial change. The texture updater is currently used for appending updates before the LRC is even created.

Hmm, I don't think this is true - we currently can't do any painting work until we initialize the LRC because we need to know the MAX_TEXTURE_SIZE before we can figure out what our tile size is.  So I think we always initialize the LRC before we hit any code that could want to queue texture updates.

[James Robinson]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (From update of attachment 147755 [details] [details] [details])
> > > In particular, I think we should remove the updatespending state on the state machine and have the current updater be *owned* by LRC. Commit can set it directly and the proxy will just tick it. This fixes the ownership issues and allows us to have a considerably simper design I think.
> > 
> > I'd like the updatesPending state change to land separately and I don't mind if that's done before we land this patch if that's what you prefer.
> > 
> > Making the current texture updater *owned* by LRC is not a trivial change. The texture updater is currently used for appending updates before the LRC is even created.
> 
> Hmm, I don't think this is true - we currently can't do any painting work until we initialize the LRC because we need to know the MAX_TEXTURE_SIZE before we can figure out what our tile size is.  So I think we always initialize the LRC before we hit any code that could want to queue texture updates.

I checked and it looks like my memory is correct - we _always_ initialize the LRC before doing any updates.

[David Reveman]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (From update of attachment 147755 [details] [details] [details])
> > > In particular, I think we should remove the updatespending state on the state machine and have the current updater be *owned* by LRC. Commit can set it directly and the proxy will just tick it. This fixes the ownership issues and allows us to have a considerably simper design I think.
> > 
> > I'd like the updatesPending state change to land separately and I don't mind if that's done before we land this patch if that's what you prefer.
> > 
> > Making the current texture updater *owned* by LRC is not a trivial change. The texture updater is currently used for appending updates before the LRC is even created.
> 
> Hmm, I don't think this is true - we currently can't do any painting work until we initialize the LRC because we need to know the MAX_TEXTURE_SIZE before we can figure out what our tile size is.  So I think we always initialize the LRC before we hit any code that could want to queue texture updates.

Yes, sorry. I got that a bit wrong. We haven't actually started appending any updates to the updater at the time we initialize the LRC. We instantiate the updater on the impl thread before posting a beginFrame. Execution of beginFrame() on the main thread then calls CCLayerTreeHost::updateLayers(updater) with the updater and that will cause the LRC to be initialized.

So we could potentially make the LRC own the updater and still append updates to it. Will require a bit of refactoring to CCThreadProxy and LTH though. Is this what we want? It seems unnecessarily complicated to pass the texture updater pointer to beginFrame just for it to append updates to it. Maybe just have the main thread produce a vector of updates and pass it to the impl thread with beginFrameComplete?

[Nat Duca]

Instead of having the proxy own the updater, lets make it ephemral.

beginFrame *allocates* a new updater every time it runs and passes it to beginFrameCompleteOnImplThread. Then, we have LRC ownptr an updater which is initially null. The bfacOnImplThread passownptr's its updater to the LRC.

Et voila.

[David Reveman]

(In reply to comment #31)
> Instead of having the proxy own the updater, lets make it ephemral.
> 
> beginFrame *allocates* a new updater every time it runs and passes it to beginFrameCompleteOnImplThread. Then, we have LRC ownptr an updater which is initially null. The bfacOnImplThread passownptr's its updater to the LRC.
> 
> Et voila.

OK, that's close to what my current patch is doing. beginFrame allocates a new updater every time it runs and passes it to beginFrameCompleteOnImplThread.

The difference is beginFrameCompleteOnImplThread in my patch then passes textureupdater to the constructor of CCTextureUpdateController along with GC3D, TextureCopier, Uploader, Allocator, etc which are needed to perform updates.

So unless we keep the CCTextureUpdateController class, I'd need to add some way to pass GC3D, TextureUploader and such to the texture updater as those are not available on the main thread where the updater is created.

I think I prefer the CCTextureUpdateController class over setters for GC3D, TextureCopier, etc on the texture updater.

[Nat Duca]

(In reply to comment #32)
> 
> I think I prefer the CCTextureUpdateController class over setters for GC3D, TextureCopier, etc on the texture updater.

Disagree. I think we should late-add these functions when the texture updater gets bound to a lrc. It has no need for those pointers until then, and (in fact) use of those pointers is unsafe on the main thread at all times.

[Nat Duca]

When you get this on the LRC, then I think can fold this all into a single virtual TextureUpdater class that has two subclasses -- one for streaming and one for old-style. I vastly prefer this to 4 classes as is currently proposed.

[David Reveman]

(In reply to comment #33)
> (In reply to comment #32)
> > 
> > I think I prefer the CCTextureUpdateController class over setters for GC3D, TextureCopier, etc on the texture updater.
> 
> Disagree. I think we should late-add these functions when the texture updater gets bound to a lrc. It has no need for those pointers until then, and (in fact) use of those pointers is unsafe on the main thread at all times.

Not sure you get my point. Using any of these pointers on the main thread is of course unsafe.

I think it's a bad idea to expose the same interface to the painting system on the main thread as the update system on the impl thread. If we just add timing code to CCTextureUpdater and you no longer provide a GC3D pointer to the CCTextureUpdater::update() function, the only way to know that calling update() on the main thread is invalid is by looking at the details of the CCTextureUpdater implementation!

I think the best approach is to expose one interface on the main thread (lets call it TextureUpdaterQueue) with append functions and another on the impl thread (TextureUpdater) with update functions.

We can either have CCTextureUpdater implement both interfaces or we can have separate instances and pass the ownership of the TextureUpdaterQueue instance to the TextureUpdater during beginFrameComplete. I like the later as it allows us to pass the CCTimer and the GC3D reference to the TextureUpdater constructor rather than late-add them using setters.

My current patch does this but to keep the size of the patch reasonable: TextureUpdaterQueue=CCTextureUpdater, TextureUpdater=CCTextureUpdateController and the update(GC3D*) function is kept in the CCTextureUpdater class so that the changes to the single thread proxy and unit tests are minimal.

Whatever we decide is the right approach, I think the re-factoring should land prior to this patch.

[David Reveman]

Created attachment 155294 [details]
Patch

New patch that depends on CCTextureUpdater re-factoring.

[David Reveman]

Created attachment 156769 [details]
Patch

Rebase.

[David Reveman]

Created attachment 156832 [details]
Patch

Rebase

[Adrienne Walker]

Comment on attachment 156832 [details]
Patch

I'm a little confused about who should be responsible for the timing part of this, but maybe I don't understand the state machine enough.  I would have thought that the state machine would just spin on ACTION_BEGIN_UPDATE_MORE_RESOURCES, rather than having a CCTimer in the texture update controller controlling its own scheduling (in a way).  Passing a time to the controller to abort sounds good to me, but I'm having trouble understanding how the outer loop should be controlled.

nduca: Can you give this a review?

[Brian Anderson]

Comment on attachment 156832 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:148
> +    return WTF::monotonicallyIncreasingTime();

John is working on a patch that will make CCDelayBasedTimeSource use a highResolutionMonotonicallyIncreasing time, which will not be compatible with this.  When this should also use highResolutionMonotonicallyIncreasing will depend on when https://bugs.webkit.org/show_bug.cgi?id=93316 lands.

[David Reveman]

(In reply to comment #40)
> (From update of attachment 156832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156832&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:148
> > +    return WTF::monotonicallyIncreasingTime();
> 
> John is working on a patch that will make CCDelayBasedTimeSource use a highResolutionMonotonicallyIncreasing time, which will not be compatible with this.  When this should also use highResolutionMonotonicallyIncreasing will depend on when https://bugs.webkit.org/show_bug.cgi?id=93316 lands.

OK, thanks for the heads up. Seems like a simple adjustment that I'll make sure to get done if 93316 lands before this patch.

[David Reveman]

Created attachment 157811 [details]
Patch

Rebase

[Nat Duca]

Comment on attachment 157811 [details]
Patch

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

This looks good, I like it. A few questions...

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:152
> +    return m_timeSource->nextTickTime();

how does this work in the case where throttling is off? E.g. vsync disabled?

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:178
>                  m_updateMoreResourcesPending = true;

are we still using this bool?

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:180
> +                m_stateMachine.beginUpdateMoreResourcesComplete(false);

when do we set it to true?

[David Reveman]

(In reply to comment #43)
> (From update of attachment 157811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157811&action=review
> 
> This looks good, I like it. A few questions...
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:152
> > +    return m_timeSource->nextTickTime();
> 
> how does this work in the case where throttling is off? E.g. vsync disabled?

Just about to upload a new patch that handles this properly. I made CCFrameRateController::nextTickTime() return WTF::monotonicallyIncreasingTime() when vsync is disabled and adjusted the CCTextureUpdateController to complete uploads in a finite amount of time which is something we needed anyhow.

https://bugs.webkit.org/show_bug.cgi?id=94175 needs to be fixed for disabled vsync to work properly but that's separate issue.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:178
> >                  m_updateMoreResourcesPending = true;
> 
> are we still using this bool?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:180
> > +                m_stateMachine.beginUpdateMoreResourcesComplete(false);
> 
> when do we set it to true?

m_updateMoreResourcesPending is used to issue a m_stateMachine.beginUpdateMoreResourcesComplete(m_client->hasMoreResourceUpdates()) call when we receive a vsync tick. We've gone back and worth on this before and the decision was to not have the state machine API changes required to remove these variables as part of this patch. I think that's good idea. We can of course follow up with a patch that cleans up the state machine API after landing this.

[David Reveman]

Created attachment 158699 [details]
Patch

Support for disabled vsync and make updating complete in a finite amount of time.

[Nat Duca]

Comment on attachment 158699 [details]
Patch

Makes sense to me. Thanks for refreshing my memory. Enne, this LGTM if you want to give it the official stamp of goodness plus any nitting you might have.

[James Robinson]

Comment on attachment 158699 [details]
Patch

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

R=me, but the time faking stuff is due for a cleanup. It's hard to reason about what clock different systems are using.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:155
> +    return WTF::monotonicallyIncreasingTime();

no WTF:: - monotonicallyIncreasingTime() is in the global namespace and not overridden by anything here

you should also move the <wtf/CurrentTime.h> #include from CCFRC.h to CCFRC.cpp - I've no idea why it would be in the header.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:58
> +    virtual double monotonicallyIncreasingTime() const;

we really should pick a new name rather than aliasing a global symbol

[David Reveman]

Created attachment 158863 [details]
Patch

Rename monotonicallyIncreasingTime() to monotonicTimeNow() and move wtf/CurrentTime.h include to CCFRC.cpp

[WebKit Review Bot]

Comment on attachment 158863 [details]
Patch

Clearing flags on attachment: 158863

Committed r125800: <http://trac.webkit.org/changeset/125800>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Reverted r125800 for reason:

Hypothesis that this change caused gpu_throughput_tests to start timing out on all platforms on the Chromium GPU canaries.

Committed r125843: <http://trac.webkit.org/changeset/125843>

[David Reveman]

Created attachment 159255 [details]
Patch

Rebase

[WebKit Review Bot]

Comment on attachment 159255 [details]
Patch

Clearing flags on attachment: 159255

Committed r126055: <http://trac.webkit.org/changeset/126055>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Labour]

Comment on attachment 159255 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:159
> +    return monotonicallyIncreasingTime();

I think this is wrong and causes things to break scheduling if !m_isTimeSourceThrottling.
Down in CCTextureUpdateController::updateMoreTexturesIfEnoughTimeRemaining this causes hasTimeRemaining to always be false, so we never start uploading until the next vsync tick.

[David Reveman]

(In reply to comment #55)
> (From update of attachment 159255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159255&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:159
> > +    return monotonicallyIncreasingTime();
> 
> I think this is wrong and causes things to break scheduling if !m_isTimeSourceThrottling.
> Down in CCTextureUpdateController::updateMoreTexturesIfEnoughTimeRemaining this causes hasTimeRemaining to always be false, so we never start uploading until the next vsync tick.

This is currently the expected behavior. Not having any updates initiated until we get a vsync tick might seem a bit awkward but shouldn't have any real effect as there shouldn't be any delay until the next vsync tick when !m_isTimeSourceThrottling.

Do you think the problem you're seeing is this https://bugs.webkit.org/show_bug.cgi?id=94645 ? That problem existed before this patch landed but probably much more likely to be causing problems now.

[Sami Kyöstilä]

Created attachment 159954 [details]
Chromium trace showing frame throttling

It looks like this patch is throttling all animations (e.g., scrolling, requestAnimationFrame) to half of the nominal frame rate. For example, see http://diden.net/test/cycle16-raf.html, or try scrolling the page with the compositor fps graph visible. Attached is a trace I took while scrolling a page via the scrollbar.

[David Reveman]

(In reply to comment #57)
> Created an attachment (id=159954) [details]
> Chromium trace showing frame throttling
> 
> It looks like this patch is throttling all animations (e.g., scrolling, requestAnimationFrame) to half of the nominal frame rate. For example, see http://diden.net/test/cycle16-raf.html, or try scrolling the page with the compositor fps graph visible. Attached is a trace I took while scrolling a page via the scrollbar.

Thanks for the example. The problem is https://bugs.webkit.org/show_bug.cgi?id=94719 and the fix attached to that bug seem to get the frame rate to where it should be.