1669239 - Schedule the next tick to paint in PuppetWidget::Paint instead of painting directly

Status: RESOLVED FIXED

(Core :: Performance, defect, P3)

Description

[Sean Feng [:sefeng]]

The WillPaintWindow call is problematic for FirstContentfulPaint
algorithm because it causes paints that outside of ticks, which would
revoke the viewer flush for the next tick, hence no firstContentful
paint entries are fired.

The proposed solution here is instead of doing the paint directly in
PuppetWidget::Paint, we ask the next tick to do it.

Comment 1

[Sean Feng [:sefeng]]

Attachment: Bug 1669239 - Call Tick to paint in PuppetWidget::Paint instead of using WillPaintWindow

The WillPaintWindow call is problematic for FirstContentfulPaint
algorithm because it causes paints that outside of ticks, which would
revoke the viewer flush for the next tick, hence no firstContentful
paint entries are fired.

However, this WillPaintWindow call is also useful because it allows
the parent to aware that the layers are updated, so that the parent
can fire corresponding events based on the status. If we don't do it,
parent may end up not knowing that the layers are updated, so some
tests may stall because of the missing events.

The proposed solution here is instead of doing the paint directly in
PuppetWidget::Paint, we ask the next tick to do it.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/604e2561311f
Call Tick to paint in PuppetWidget::Paint instead of using WillPaintWindow r=emilio,mattwoodrow

Comment 3

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/604e2561311f

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Sean, there's an improvement that seems to be caused by your patch, but around this revision it's also a build failure that prevents me from figuring out for certain what's the paych that caused the improvement. Does the improvement below makes sense to be caused by this patch?
== Change summary for alert #27381 (as of Mon, 26 Oct 2020 21:12:38 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
4%	tresize		macosx1014-64-shippable	e10s stylo	13.02 -> 12.52

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27381

Comment 5

[Sean Feng [:sefeng]]

Yeah, the test basically measures the time for window.resizeTo by using MozAfterPaint event, which is indeed something that this patch can impact. Ahthough I am a bit suprised of the improvement, but I think it's possible.

Comment 6

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Actually, if I look more closely, there's a slight improvement in results and the graph became bimodal.

Comment 7

[Alexandru Ionescu (needinfo me) [:alexandrui]]

So I tend ore to say that the improvement might be a regression in terms of graph stability. What do you say? Does this make sense to you?

Comment 8

[Sean Feng [:sefeng]]

Yeah, it makes sense to call it an regression in terms of graph stability. This makes sense since the patch started to schedule a paint at where it was painting directly.

I am not worrying about it since the overall performance is better. Would it be okay for you?

Comment 9

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Sorry for replying this late. I am a bit worried as the noise is one of the main factors that are preventing us to identify the regression cause.:sefen

Comment 10

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Wait, did this test stopped running on autoland since more than a month ago?

Comment 11

[Sean Feng [:sefeng]]

This patch indeed makes the paint from sync to async, so it makes sense when the tests become nosier. I am not sure if there are things that we can do to improve the graph stability.

Alexandru, the links seem to be broken so I can't check the current status.

Comment 12

[Alexandru Ionescu (needinfo me) [:alexandrui]]

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&series=autoland,2056535,1,1&timerange=5184000&zoom=1602225032387,1604469402591,11.638130248281701,14.13227760840817

Comment 13

[Sean Feng [:sefeng]]

I think I get what you meant. Are you worrying that we might not be able to identify future regressions due to the test becomes nosier? I wish we can still identify regressions for nosier benchmarks....

Honestly, I don't know if there are things that we can do to improve it. What do we usually do for cases like this?

Comment 14

[Alexandru Ionescu (needinfo me) [:alexandrui]]

The performance team is trying to stick the sheriffed tests as stable as possible. A stable test means also a healthy one. I'm not sure what you can do, cases like this don't happen very often and there's no procedure for them. I would look for way to reduce noise, maybe a better approach for the code.
Can you explain is a simple way what is this patch doing, please? My gut feeling is that there's a tradeoff (well, it's always a tradeoff, haha).

Comment 15

[Sean Feng [:sefeng]]

So PuppetWidget::Invalidate was a method that was being used for tab switches and tab resizes. It would make a paint and this paint was a prerequisite for a bunch of painting related events to fire. These events were used in many places, I believe this particular test uses MozAfterPaint.

The issue with this approach was that this paint was executed outside of the Tick, so it messed up first-contentful-paint timing because the spec required paints to happen within the Tick.

So instead of doing paints directly from PuppetWidget::Invalidate, this patch changed it to schedule paints to run at the next Tick by scheduling a Tick.

Since this patch actually scheduled paints to run later than before, it's not clear to me how the performance got improved. I wonder if it's caused by the scheduled paint actually makes MozAfterPaint to be fired earlier because we started to schedule a tick earlier.

Does it sound good?