1382132 - Improving throttling GenerateFrame()

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Sotaro Ikeda [:sotaro]]

GenerateFrame() is already throttled by Bug 1365196. It is better to be improved, since current code permits that multiple pending GenerateFrame()s are queued.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: temporal patch - print duration from GenerateFrame until end of RendererOGL::Render()

Comment 2

[Sotaro Ikeda [:sotaro]]

The following code throttles GenerateFrame().

> const uint32_t maxPendingFrameCount = 2;
>
>  if (!mForceRendering &&
>      wr::RenderThread::Get()->GetPendingFrameCount(mApi->GetId()) > maxPendingFrameCount) {
>    // Render thread is busy, try next time.
>    ScheduleComposition();
>    return;
>  }

  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/wr/WebRenderBridgeParent.cpp#1089


"maxPendingFrameCount = 2" in current gecko. During scrolling https://www.yahoo.co.jp/, the duration was the following.
  - 35 ms - 40ms [on my linux desktop pc]
  - 160 ms - 200ms [on my win10 laptop pc]

When "maxPendingFrameCount = 1", the duraiton was
  - 20ms [on my linux desktop pc]
  - 130 ms - 150ms [on my win10 laptop pc]

When "maxPendingFrameCount = 0", the duraiton was
  - 10 ms - 15ms[ [on my linux desktop pc]
  - 30 ms - 60ms[on my win10 laptop pc]

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce maxPendingFrameCount to 1

Comment 4

[Sotaro Ikeda [:sotaro]]

attachment 8888197 [details] [diff] [review] reduce the duration of Comment 2. But it works well the scrolling of scrolling https://www.yahoo.co.jp/.

But the patch does not works well like https://jrmuizel.github.io/perf-tests/moving-balls.html of https://github.com/servo/webrender/issues/1501. In this case, a lot of ApiMsg::GenerateFrame was queued.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Improve throttling GenerateFrame()

Comment 6

[Sotaro Ikeda [:sotaro]]

With Bug 1382141, WebRenderBridgeParent::CompositeToTarget() becomes only place to trigger ApiMsg::GenerateFrame message.
Then IncPendingFrameCount() could be called in the CompositeToTarget(). By it, RenderThread::GetPendingFrameCount() could track PendingFrameCount correclty.

Comment 7

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20fe6f5ac58e3eaa053d1e3ca1b9a280cb41dcb6

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8888214 [details] [diff] [review]
patch - Improve throttling GenerateFrame()

Review of attachment 8888214 [details] [diff] [review]:
-----------------------------------------------------------------

So if I understand correctly, this patch makes it so that we don't schedule another frame if there is already one frame being processed by webrender (including rendr backend and renderer work).
This means we can't pipeline the rendering of frame N in the renderer while frame N+1 is being built on the render backend, and it means that our 16ms frame budget is the sum of the building and rendering the frame (while if we allowed pipelining the renderer and the render backend we could allow 16ms for building and 16ms for rendering at the cost of an extra frame of latency.

I think that it's worth having a discussion with the rest of the team about whether or not to allow pipelining. In the mean time r+, but we should revisit this and confirm at some point.

Comment 9

[Nicolas Silva [:nical]]

> When "maxPendingFrameCount = 0", the duraiton was
>  - 10 ms - 15ms[ [on my linux desktop pc]
>  - 30 ms - 60ms[on my win10 laptop pc]

What are you measuring exactly?

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #9)
> > When "maxPendingFrameCount = 0", the duraiton was
> >  - 10 ms - 15ms[ [on my linux desktop pc]
> >  - 30 ms - 60ms[on my win10 laptop pc]
> 
> What are you measuring exactly?


"maxPendingFrameCount = 0" actually means, max pending FrameCount is one, because of current implementation.

>  if (!mForceRendering &&
>      wr::RenderThread::Get()->GetPendingFrameCount(mApi->GetId()) > (maxPendingFrameCount = 0)) {


attachment 8888214 [details] [diff] [review] changes the above to 

>  if (!mForceRendering &&
>      wr::RenderThread::Get()->GetPendingFrameCount(mApi->GetId()) >= (maxPendingFrameCount = 1)) {


And it means only one frame is pending from mApi->GenerateFrame() until end of RendererOGL::Render(). And the following measured duration from mApi->GenerateFrame() until end of RendererOGL::Render() during scrolling https://www.yahoo.co.jp/.

> >  - 10 ms - 15ms[ [on my linux desktop pc]
> >  - 30 ms - 60ms[on my win10 laptop pc]

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #8)
> Comment on attachment 8888214 [details] [diff] [review]
> patch - Improve throttling GenerateFrame()
> 
> Review of attachment 8888214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So if I understand correctly, this patch makes it so that we don't schedule
> another frame if there is already one frame being processed by webrender
> (including rendr backend and renderer work).
> This means we can't pipeline the rendering of frame N in the renderer while
> frame N+1 is being built on the render backend, and it means that our 16ms
> frame budget is the sum of the building and rendering the frame (while if we
> allowed pipelining the renderer and the render backend we could allow 16ms
> for building and 16ms for rendering at the cost of an extra frame of latency.

As in comment 2 and comment 4, when I tested locally, limit only one pending frame like attachment 8888214 [details] [diff] [review] minimize the rendering latency in all situations.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #8)
> 
> I think that it's worth having a discussion with the rest of the team about
> whether or not to allow pipelining. In the mean time r+, but we should
> revisit this and confirm at some point.

Yea, it is reasonable:)

Comment 13

[Sotaro Ikeda [:sotaro]]

It seems better to measure from EndTransaction until until end of RendererOGL::Render(). It is going to be handled by Bug 1377619.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e74d958657e
Improving throttling GenerateFrame() r=nical

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e74d958657e