Closed Bug 1382132 Opened 2 years ago Closed 2 years ago

Improving throttling GenerateFrame()

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 1 obsolete file)

GenerateFrame() is already throttled by Bug 1365196. It is better to be improved, since current code permits that multiple pending GenerateFrame()s are queued.
Assignee: nobody → sotaro.ikeda.g
See Also: → 1377619
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]
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.
Depends on: 1382141
Attachment #8888197 - Attachment is obsolete: true
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.
Attachment #8888214 - Attachment description: Improving throttling GenerateFrame() → patch - Improve throttling GenerateFrame()
Attachment #8888214 - Flags: review?(nical.bugzilla)
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.
Attachment #8888214 - Flags: review?(nical.bugzilla) → review+
> 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?
(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]
(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.
(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:)
It seems better to measure from EndTransaction until until end of RendererOGL::Render(). It is going to be handled by Bug 1377619.
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e74d958657e
Improving throttling GenerateFrame() r=nical
https://hg.mozilla.org/mozilla-central/rev/7e74d958657e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.