Develop a continuous paint mode for Firefox

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

({feature})

unspecified
mozilla51
feature
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Similar to Chrome - https://developers.google.com/web/updates/2013/02/Profiling-Long-Paint-Times-with-DevTools-Continuous-Painting-Mode - and WebRender, develop a continuous paint preference which always invalidates the whole page and renders the whole page every frame. This will allow easy measurement of paint performance with comparison to Chrome and WebRender.

Ideally we'd like to measure two things:
1) Paint rasterization time
2) Composite time.

Maybe a third later of GPU time to be equivalent to WebRender.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Whiteboard: gfx-noted
(Assignee)

Comment 1

2 years ago
Created attachment 8780744 [details] [diff] [review]
Add a pref for continuous paint mode

For content processes and if the pref is enabled, we invalidate all layers and measure the time it takes to rasterize content. If the pref is enabled, we also always tick the refresh driver to force paints. We then ship this data over IPC to the compositor and draw a little counter that measures both the time it took to paint the content as well as composite. To draw the actual numbers, we create a data source surface, write the numbers into the surface with skia, and update the compositor to composite the surface.
Attachment #8780744 - Flags: review?(matt.woodrow)
Comment on attachment 8780744 [details] [diff] [review]
Add a pref for continuous paint mode

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

I assume this is useful for what you need, but isn't it easy to miss long paint numbers if they get overwritten 16ms later?

Should we consider drawing a graph instead so that we can display the last N frames worth of timings at once?

::: gfx/layers/composite/FPSCounter.cpp
@@ +394,4 @@
>  
>      Rect drawRect = Rect(aOffsetX + n * FontWidth, aOffsetY, FontWidth, FontHeight);
>      IntRect clipRect = IntRect(0, 0, 300, 100);
> +    aCompositor->DrawQuad(drawRect, clipRect, aEffectChain, opacity, transform);

Would be nice to combine FPSCounter and PaintCounter into a single thing at some point in the future.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +573,5 @@
>      aInvalidRegion.Or(aInvalidRegion, nsIntRect(0, 0, 10, aBounds.height));
>    }
> +  if (drawPaintTimes) {
> +    IntSize paintSize = PaintCounter::GetSize();
> +    aInvalidRegion.Or(aInvalidRegion, nsIntRect(0, 0, paintSize.width, paintSize.height));

Isn't this missing the 170 pixel Y offset?

How about making PaintCounter::GetSize() a non-static function that returns an IntRect()? Then we can return the actual runtime size and position that we need to invalidate.

The (300,60) default size can move to a static var within PaintCounter.cpp (probably just a single static IntRect?) and we can remove the size parameter from the constructor.

@@ +596,4 @@
>    bool drawFps = gfxPrefs::LayersDrawFPS();
>    bool drawFrameCounter = gfxPrefs::DrawFrameCounter();
>    bool drawFrameColorBars = gfxPrefs::CompositorDrawColorBars();
> +  bool drawPaintTimes = gfxPrefs::AlwaysPaint();

Might be nice to allow turning on the drawing of paint times without triggering extra paints.

::: gfx/layers/composite/PaintCounter.cpp
@@ +47,5 @@
> +void
> +PaintCounter::Draw(Compositor* aCompositor, TimeDuration aPaintTime, TimeDuration aCompositeTime) {
> +  const int buffer_size = 32;
> +  char buffer[buffer_size];
> +  snprintf(buffer, buffer_size, "P: %.2f  C: %.2f",

Can we end up running out of space in the buffer for the composite time if the paint time is too long? Do we care?

@@ +73,5 @@
> +  EffectChain effectChain;
> +  effectChain.mPrimaryEffect = mTexturedEffect;
> +
> +  // Position the numbers right below chrome
> +  Rect drawRect = Rect(0, 175, mSize.width, mSize.height);

Can you move the positioning offset into a const var at the top of the file please.

::: gfx/layers/ipc/PLayerTransaction.ipdl
@@ +58,4 @@
>                int32_t paintSyncId)
>      returns (EditReply[] reply);
>  
> +  async PaintTime(uint64_t id, TimeDuration paintTime);

Any reason not to just include this as an extra parameter to Update(NoSwap)?
Attachment #8780744 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 3

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Comment on attachment 8780744 [details] [diff] [review]
> Add a pref for continuous paint mode
> 
> Review of attachment 8780744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume this is useful for what you need, but isn't it easy to miss long
> paint numbers if they get overwritten 16ms later?

Yeah, ATM I'm just also printing things to the console locally.

> 
> Should we consider drawing a graph instead so that we can display the last N
> frames worth of timings at once?

That's a good idea. Both WebRender and Chrome do that, except the graph is really hard
to read since so many frames go by in a second. I'm still not sure what a great way
to present the data is.

> 
> How about making PaintCounter::GetSize() a non-static function that returns
> an IntRect()? Then we can return the actual runtime size and position that
> we need to invalidate.
> 
> The (300,60) default size can move to a static var within PaintCounter.cpp
> (probably just a single static IntRect?) and we can remove the size
> parameter from the constructor.

Yeah I just made a single static IntRect instead, thanks

> 
> @@ +596,4 @@
> >    bool drawFps = gfxPrefs::LayersDrawFPS();
> >    bool drawFrameCounter = gfxPrefs::DrawFrameCounter();
> >    bool drawFrameColorBars = gfxPrefs::CompositorDrawColorBars();
> > +  bool drawPaintTimes = gfxPrefs::AlwaysPaint();
> 
> Might be nice to allow turning on the drawing of paint times without
> triggering extra paints.

Yeah, but I didn't want to make two prefs yet.

> 
> ::: gfx/layers/composite/PaintCounter.cpp
> @@ +47,5 @@
> > +void
> > +PaintCounter::Draw(Compositor* aCompositor, TimeDuration aPaintTime, TimeDuration aCompositeTime) {
> > +  const int buffer_size = 32;
> > +  char buffer[buffer_size];
> > +  snprintf(buffer, buffer_size, "P: %.2f  C: %.2f",
> 
> Can we end up running out of space in the buffer for the composite time if
> the paint time is too long? Do we care?

I made it 48.

> 
> @@ +73,5 @@
> > +  EffectChain effectChain;
> > +  effectChain.mPrimaryEffect = mTexturedEffect;
> > +
> > +  // Position the numbers right below chrome
> > +  Rect drawRect = Rect(0, 175, mSize.width, mSize.height);
> 
> Can you move the positioning offset into a const var at the top of the file
> please.

Moved this into a single static IntRect.

> 
> ::: gfx/layers/ipc/PLayerTransaction.ipdl
> @@ +58,4 @@
> >                int32_t paintSyncId)
> >      returns (EditReply[] reply);
> >  
> > +  async PaintTime(uint64_t id, TimeDuration paintTime);
> 
> Any reason not to just include this as an extra parameter to Update(NoSwap)?

I didn't really want to always send a TimeDuration parameter to Update on every
layer transaction just in case someone uses this pref. Do you have a strong preference for that?
(Assignee)

Comment 4

2 years ago
Created attachment 8781250 [details] [diff] [review]
Add a pref for continuous paint mode

Updated with feedback from comment 2.
Attachment #8780744 - Attachment is obsolete: true
Attachment #8781250 - Flags: review+
(In reply to Mason Chang [:mchang] from comment #3)
 
> That's a good idea. Both WebRender and Chrome do that, except the graph is
> really hard
> to read since so many frames go by in a second. I'm still not sure what a
> great way
> to present the data is.

One option might be to just print a frame id number on screen, and then have about:painttiming that shows all of the recorded data for each frame id.


> 
> I didn't really want to always send a TimeDuration parameter to Update on
> every
> layer transaction just in case someone uses this pref. Do you have a strong
> preference for that?

Nah, this way is fine, was just curious.
(Assignee)

Comment 6

2 years ago
Try looks ok I think - https://treeherder.mozilla.org/#/jobs?repo=try&revision=8360c3f3dcbe - Always so hard to tell now :/

Comment 7

2 years ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaaab9a073a4
Develop a continuous paint mode for Firefox. r=mattwoodrow
This was backed out by https://hg.mozilla.org/integration/mozilla-inbound/rev/1b51661678d5 but it didn't list a bug number so pulsebot didn't update this bug.

Comment 9

2 years ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/635d266614ad
Develop a continuous paint mode for Firefox. r=mattwoodrow
(Assignee)

Comment 10

2 years ago
This failed the first inbound but succeeded on try because bug 1293384 was on inbound, which renamed snprintf.h.

Comment 11

2 years ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5776989b4c1
Backed out changeset 635d266614ad for windows build failures. r=me. CLOSED TREE

Comment 12

2 years ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8824b7a86039
Develop a continuous paint mode for Firefox. r=mattwoodrow

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/635d266614ad
https://hg.mozilla.org/mozilla-central/rev/a5776989b4c1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Comment 14

2 years ago
(In reply to Wes Kocher (:KWierso) from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/635d266614ad
> https://hg.mozilla.org/mozilla-central/rev/a5776989b4c1

The second one is the backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8824b7a86039
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Mason, is this something manual QA could help test?
Flags: qe-verify?
Flags: needinfo?(mchang)
(Assignee)

Comment 17

2 years ago
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #16)
> Mason, is this something manual QA could help test?

No, this is just a gfx developer tool. Thanks though.
Flags: needinfo?(mchang)
(In reply to Mason Chang [:mchang] from comment #17)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #16)
> > Mason, is this something manual QA could help test?
> 
> No, this is just a gfx developer tool. Thanks though.

Thank you for following up on this. Setting the QE flag accordingly.
Flags: qe-verify? → qe-verify-
Depends on: 1319374
You need to log in before you can comment on or make changes to this bug.