cc: Remove resource updates from scheduler.

cc: Remove resource updates from scheduler.

Perform all buffered resource updates before calling
CCScheduler::beginFrameComplete. Non-buffered updates are done
at the same time as commit.

BUG=149194
TEST=cc_unittests

[reveman, 2012-09-14 15:42:51]

Hi,

this removes resource updates from scheduler. The current patch doesn't remove
the old resource update state. It's just not using it. I'd like to remove all
unused code before landing but the current patch should be good for testing the
performance impact from this change.

[jamesr, 2012-09-14 23:03:06]

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController.cpp
File cc/CCTextureUpdateController.cpp (right):

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:13: using namespace std;
chromium and WebKit style is to prefer std::foo instead of this using directive

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:151:
m_timer->startOneShot(uploaderBusyTickRate);
why the new 1ms delay? did anything in the previous system tick this fast?

[reveman, 2012-09-14 23:38:00]

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController.cpp
File cc/CCTextureUpdateController.cpp (right):

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:13: using namespace std;
On 2012/09/14 23:03:06, jamesr wrote:
> chromium and WebKit style is to prefer std::foo instead of this using
directive

Hm, I see a lot of the using directive in cc. I thought we preferred this at
least in WebKit. Anyhow, I'll fix this.

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:151:
m_timer->startOneShot(uploaderBusyTickRate);
On 2012/09/14 23:03:06, jamesr wrote:
> why the new 1ms delay? did anything in the previous system tick this fast?

So the main reason this code is here is because we don't want to consider full
uploads done until the uploader is available. If we didn't make sure the
uploader was available, we would transition to commit and unnecessarily block
there while waiting for the uploader to become available so we can perform
partial uploads.

we could use the tick rate as delay here but using a shorter delay significantly
reduces the amount of unnecessary idle time on the impl during the
beginFrameComplete to commit cycle.

I'll add a comment to the code here if nothing else.

[jamesr, 2012-09-17 06:57:38]

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController.cpp
File cc/CCTextureUpdateController.cpp (right):

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:13: using namespace std;
On 2012/09/14 23:38:00, David Reveman wrote:
> On 2012/09/14 23:03:06, jamesr wrote:
> > chromium and WebKit style is to prefer std::foo instead of this using
> directive
> 
> Hm, I see a lot of the using directive in cc. I thought we preferred this at
> least in WebKit. Anyhow, I'll fix this.

This changed in WebKit several months ago:
http://www.webkit.org/coding/coding-style.html#using-in-cpp . A fair amount of
code is still using the old WebKit style, but we shouldn't propagate it further.

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:151:
m_timer->startOneShot(uploaderBusyTickRate);
On 2012/09/14 23:38:00, David Reveman wrote:
> On 2012/09/14 23:03:06, jamesr wrote:
> > why the new 1ms delay? did anything in the previous system tick this fast?
> 
> So the main reason this code is here is because we don't want to consider full
> uploads done until the uploader is available. If we didn't make sure the
> uploader was available, we would transition to commit and unnecessarily block
> there while waiting for the uploader to become available so we can perform
> partial uploads.
> 
> we could use the tick rate as delay here but using a shorter delay
significantly
> reduces the amount of unnecessary idle time on the impl during the
> beginFrameComplete to commit cycle.
> 
> I'll add a comment to the code here if nothing else.

This is all because we poll (via the query object) to see if the uploader's
ready instead of getting a callback, right?

I'm a bit worried about hammering the system with really fast timeouts - it adds
a lot of variability and stress to the scheduler and the rest of the system. 
What's the common case here?  I would guess a small number of full uploads
(sometimes zero) and a small number of partial uploads (sometimes zero).  Are we
going to end up spinning on this every time we do any buffered full uploads? 
How often do you expect this to spin?

For the case you mention of partial uploads I think as soon as we decide to do
partial uploads there's not much point blocking on the client side - if we're at
the point where we're ready to upload partials we should just go ahead and
buffer the frame up.  We can't really usefully put any frames up with the old
textures once we've begun the partial uploads.

https://codereview.chromium.org/10933095/diff/1/cc/CCThreadProxy.cpp
File cc/CCThreadProxy.cpp (right):

https://codereview.chromium.org/10933095/diff/1/cc/CCThreadProxy.cpp#newcode612
cc/CCThreadProxy.cpp:612: void
CCThreadProxy::scheduledActionUpdateMoreResources(double)
why is this function still here? what's it do?

[reveman, 2012-09-17 15:32:52]

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController.cpp
File cc/CCTextureUpdateController.cpp (right):

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:13: using namespace std;
On 2012/09/17 06:57:38, jamesr wrote:
> On 2012/09/14 23:38:00, David Reveman wrote:
> > On 2012/09/14 23:03:06, jamesr wrote:
> > > chromium and WebKit style is to prefer std::foo instead of this using
> > directive
> > 
> > Hm, I see a lot of the using directive in cc. I thought we preferred this at
> > least in WebKit. Anyhow, I'll fix this.
> 
> This changed in WebKit several months ago:
> http://www.webkit.org/coding/coding-style.html#using-in-cpp . A fair amount of
> code is still using the old WebKit style, but we shouldn't propagate it
further.

ok, clear.

https://codereview.chromium.org/10933095/diff/1/cc/CCTextureUpdateController....
cc/CCTextureUpdateController.cpp:151:
m_timer->startOneShot(uploaderBusyTickRate);
On 2012/09/17 06:57:38, jamesr wrote:
> On 2012/09/14 23:38:00, David Reveman wrote:
> > On 2012/09/14 23:03:06, jamesr wrote:
> > > why the new 1ms delay? did anything in the previous system tick this fast?
> > 
> > So the main reason this code is here is because we don't want to consider
full
> > uploads done until the uploader is available. If we didn't make sure the
> > uploader was available, we would transition to commit and unnecessarily
block
> > there while waiting for the uploader to become available so we can perform
> > partial uploads.
> > 
> > we could use the tick rate as delay here but using a shorter delay
> significantly
> > reduces the amount of unnecessary idle time on the impl during the
> > beginFrameComplete to commit cycle.
> > 
> > I'll add a comment to the code here if nothing else.
> 
> This is all because we poll (via the query object) to see if the uploader's
> ready instead of getting a callback, right?

Yes, it's because we poll.

> 
> I'm a bit worried about hammering the system with really fast timeouts - it
adds
> a lot of variability and stress to the scheduler and the rest of the system. 
> What's the common case here?  I would guess a small number of full uploads
> (sometimes zero) and a small number of partial uploads (sometimes zero).  Are
we
> going to end up spinning on this every time we do any buffered full uploads? 
> How often do you expect this to spin?

On a system where the upload estimate is accurate we rarely end up spinning
here. We do one 1ms timeout a little less than 50% of the time when performing
12+ uploads (textureUploadsPerTick=12).

In general we spin more often when upload time estimate is too aggressive and
less when it's set too conservatively.

> 
> For the case you mention of partial uploads I think as soon as we decide to do
> partial uploads there's not much point blocking on the client side - if we're
at
> the point where we're ready to upload partials we should just go ahead and
> buffer the frame up.  We can't really usefully put any frames up with the old
> textures once we've begun the partial uploads.

That's what this patch does. Partial uploads are done as part of the commit step
and we never wait for them to complete before we queue a draw.

https://codereview.chromium.org/10933095/diff/1/cc/CCThreadProxy.cpp
File cc/CCThreadProxy.cpp (right):

https://codereview.chromium.org/10933095/diff/1/cc/CCThreadProxy.cpp#newcode612
cc/CCThreadProxy.cpp:612: void
CCThreadProxy::scheduledActionUpdateMoreResources(double)
On 2012/09/17 06:57:38, jamesr wrote:
> why is this function still here? what's it do?

Nothing. As I mentioned in my initial comment, the current patch doesn't remove
the old resource update state from the scheduler but I'll do that before
landing. The reason for that was to make it easier to maintain and review this
patch. 

Btw, I think we should land this https://codereview.chromium.org/10917265/
first. It just moves partial uploads to the commit step without moving all
resource updates out of the scheduler.

[nduca, 2012-09-17 18:28:11]

Is this patch obsolete?

[reveman, 2012-09-17 23:22:24]

On 2012/09/17 18:28:11, nduca wrote:
> Is this patch obsolete?

This is still valid. I just rebased and it's even simpler now that we've already
moved partial resource updates to commit time.

[nduca, 2012-09-18 00:05:38]

It seems like you've removed awareness of nextFrameTime in addition to removing
resource updates from the scheduler.

Is there a way to get the core simplification without ignoring nextFrameTime?
Maybe add a nextFrameTime() method on the client interface?

I assume there's a followup change to remove the right bits from the state
machine?

Whats your test coverage story here now?

[reveman, 2012-09-18 00:32:56]

On 2012/09/18 00:05:38, nduca wrote:
> It seems like you've removed awareness of nextFrameTime in addition to
removing
> resource updates from the scheduler.

Yes, that's the idea. With this change we stop taking the next vsync tick time
into account when performing uploads. This CL is an implementation of what
jamesr suggested here: https://bugs.webkit.org/show_bug.cgi?id=94721#c45

I created this so we can test what kind of performance impact it has. This CL is
no high priority right now. It can wait.

> 
> Is there a way to get the core simplification without ignoring nextFrameTime?
> Maybe add a nextFrameTime() method on the client interface?

Not taking the next vsync tick time into account is the essence of the patch so
I don't think it makes sense to do this without that.

> 
> I assume there's a followup change to remove the right bits from the state
> machine?

I'm planning to include all code reduction in this patch. The CL will probably
grow by a factor of 5 by doing so and I think that will make it harder to
maintain and review so I'm waiting until we feel close to ready to land this
before taking care of all this.

> 
> Whats your test coverage story here now?

I'm mostly removing functionality with this patch. CCtextureUpdateController
test coverage should be good with added and existing unit tests. Let me know if
there's something that doesn't seem to be tested properly.

[nduca, 2012-09-18 01:08:33]

On 2012/09/18 00:32:56, David Reveman wrote:
> On 2012/09/18 00:05:38, nduca wrote:
> > It seems like you've removed awareness of nextFrameTime in addition to
> removing
> > resource updates from the scheduler.
> 
> Yes, that's the idea. With this change we stop taking the next vsync tick time
> into account when performing uploads. This CL is an implementation of what
> jamesr suggested here: https://bugs.webkit.org/show_bug.cgi?id=94721#c45
> 
> I created this so we can test what kind of performance impact it has. This CL
is
> no high priority right now. It can wait.

I like the idea of landing this patch but with the old vsync wareness stuff
still in. I think its a minor change from this. That allows us to strip out the
old stuff without making any functionality changes. Which is a good thing.


> > Is there a way to get the core simplification without ignoring
nextFrameTime?
> > Maybe add a nextFrameTime() method on the client interface?
> 
> Not taking the next vsync tick time into account is the essence of the patch
so
> I don't think it makes sense to do this without that.
I think we get the beginnings of a much-needed scheduler cleanup! It also makes
this code considerably cleaner. That on its own is a good thing and an
incremental step toward what you wanted.

> > I assume there's a followup change to remove the right bits from the state
> > machine?
> 
> I'm planning to include all code reduction in this patch. The CL will probably
> grow by a factor of 5 by doing so and I think that will make it harder to
> maintain and review so I'm waiting until we feel close to ready to land this
> before taking care of all this.

How about doing just this patch that makes that stage in the state machine a
nop. Then you can go back and remove the state machine as a change and finally
we can play with the vsync awareness.

[reveman, 2012-09-18 07:46:46]

On 2012/09/18 01:08:33, nduca wrote:
> On 2012/09/18 00:32:56, David Reveman wrote:
> > On 2012/09/18 00:05:38, nduca wrote:
> > > It seems like you've removed awareness of nextFrameTime in addition to
> > removing
> > > resource updates from the scheduler.
> > 
> > Yes, that's the idea. With this change we stop taking the next vsync tick
time
> > into account when performing uploads. This CL is an implementation of what
> > jamesr suggested here: https://bugs.webkit.org/show_bug.cgi?id=94721#c45
> > 
> > I created this so we can test what kind of performance impact it has. This
CL
> is
> > no high priority right now. It can wait.
> 
> I like the idea of landing this patch but with the old vsync wareness stuff
> still in. I think its a minor change from this. That allows us to strip out
the
> old stuff without making any functionality changes. Which is a good thing.

To do vsync aware resource updates I need the following:

1. Access to the nextVSyncTimeIfActivated() in CCTextureUpdateController.
2. A callback from the scheduler just after a draw *could* have been scheduled.
 
The second is the messy part and I currently don't see a way for us to do this
that is significantly better than the current framework. Am I missing something
obvious?

> 
> 
> > > Is there a way to get the core simplification without ignoring
> nextFrameTime?
> > > Maybe add a nextFrameTime() method on the client interface?
> > 
> > Not taking the next vsync tick time into account is the essence of the patch
> so
> > I don't think it makes sense to do this without that.
> I think we get the beginnings of a much-needed scheduler cleanup! It also
makes
> this code considerably cleaner. That on its own is a good thing and an
> incremental step toward what you wanted.
> 
> > > I assume there's a followup change to remove the right bits from the state
> > > machine?
> > 
> > I'm planning to include all code reduction in this patch. The CL will
probably
> > grow by a factor of 5 by doing so and I think that will make it harder to
> > maintain and review so I'm waiting until we feel close to ready to land this
> > before taking care of all this.
> 
> How about doing just this patch that makes that stage in the state machine a
> nop. Then you can go back and remove the state machine as a change and finally
> we can play with the vsync awareness.

[nduca, 2012-09-18 08:11:37]

For the 2nd bit, how about making a ccschedulerclient::didVsyncTick() method and
then route that into the controller? It can call didVsync unconditionally after
ticking processScheduledActions, wdyt?

[reveman, 2012-09-18 14:40:23]

On 2012/09/18 08:11:37, nduca wrote:
> For the 2nd bit, how about making a ccschedulerclient::didVsyncTick() method
and
> then route that into the controller? It can call didVsync unconditionally
after
> ticking processScheduledActions, wdyt?

That would be OK. The part I'm not sure about is how to enabling the frame rate
controller so we get these vsync ticks.

[nduca, 2012-09-18 18:38:18]

Why does it need to be enabled?

[reveman, 2012-09-18 18:57:46]

On 2012/09/18 18:38:18, nduca wrote:
> Why does it need to be enabled?

How would we otherwise get the vsync tick independent of if we need to draw?