Open Bug 1271691 Opened 8 years ago Updated 2 years ago

Adjust suppressed painting setting for a big doc

Categories

(Core :: Layout, defect)

defect

Tracking

()

People

(Reporter: jerry, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

Here is a big google doc with 200 up pages.
https://docs.google.com/document/d/1EPSmGqm2r4Qq42B4t1VOYacjTlL0JVuC8JSlUvoIhss/edit?usp=sharing

Compared with google chrome, our browser spends more time for the first google doc's toolbar showing. Before that, it only shows a white background. I think that's not a good user experience with a blank white background for a long time.

Gecko suppresses painting before it loads everything for that doc.
https://hg.mozilla.org/mozilla-central/annotate/91c4d2dc47d2/layout/base/nsPresShell.h#l668
https://hg.mozilla.org/mozilla-central/annotate/043082cb7bd8490c60815f67fbd1f33323ad7663/layout/base/nsPresShell.cpp#l1748

When I skip this suppression, I could see a boost improvement for the first content showing. I don't find the log about this suppression timer setting, but maybe it's worth to tune the timer setting for the google doc page loading performance(I mean the first content shows to users).
This is a 250ms timer, right?  Are you seeing a lag of more than about 250ms to first content showing?

When you skip the suppression, what is the tradeoff in terms of overall load time and responsiveness?
This test patch skip our painting suppression mechanism. I can see a noticeable improvement for the test doc in comment 0.
(In reply to Boris Zbarsky [:bz] from comment #1)
> This is a 250ms timer, right?  Are you seeing a lag of more than about 250ms
> to first content showing?
> 
> When you skip the suppression, what is the tradeoff in terms of overall load
> time and responsiveness?

Ye, that's a 250ms timer. But I think the main thread can't handle the 250ms timeout task because of the heavy js while loading.

I will attach some data later(e.g. the first-paint time or content-document-loaded event time).
s/can't/can/
Blocks: 1264536
Status: NEW → ASSIGNED
This patch try to check the painting suppression timer timeout value during the painting flow. Originally gecko uses a timer to set the unsuppressing task, but this timer task might be processed at the timeout timeline when we have a lot of events or tasks at main thread.
Comment on attachment 8752103 [details] [diff] [review]
update painting suppression status in painting flow. v1

I don't remove the suppression timer, but check the timeout value during painting. If it's expired, update the painting suppression status immediately.
Flags: needinfo?(bzbarsky)
Attachment #8752103 - Flags: feedback?(matt.woodrow)
That seems pretty reasonable as a general approach.
Flags: needinfo?(bzbarsky)
See Also: → 77002
Comment on attachment 8752103 [details] [diff] [review]
update painting suppression status in painting flow. v1

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

Seems reasonable to me too, do you get a noticeable improvement from doing this?
Attachment #8752103 - Flags: feedback?(matt.woodrow) → feedback+
Thanks to Cynthia, there is the video to show the difference.

https://www.youtube.com/watch?v=dgmmp0ksD_g&feature=youtu.be

The right one is the original one. The content on the left is shown faster.
Update the comparison video.
With attachment 8752103 [details] [diff] [review], the content is shown faster(about 1 second) than the original one.
Comment on attachment 8752103 [details] [diff] [review]
update painting suppression status in painting flow. v1

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

I'm not sure we could call PresShell::UnsuppressPainting() anywhere.
https://hg.mozilla.org/mozilla-central/annotate/16663eb3dcfa759f25b5e27b101bc79270c156f2/layout/base/nsPresShell.cpp#l3829

I add some checking at nsRefreshDriver::Tick() and PresShell::Paint().

And the result is in attachment 8755124 [details].
Attachment #8752103 - Flags: review?(tnikkel)
Attachment #8752103 - Flags: review?(matt.woodrow)
Hi Boris,

Is it any problem that we call additional PresShell::UnsuppressPainting() call at nsRefreshDriver::Tick() and PresShell::Paint()?
Flags: needinfo?(bzbarsky)
Comment on attachment 8752103 [details] [diff] [review]
update painting suppression status in painting flow. v1

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

Seems reasonable to me too, do you get a noticeable improvement from doing this?

::: layout/base/nsRefreshDriver.cpp
@@ +1808,5 @@
>    }
>  
>    // Recompute approximate frame visibility if it's necessary and enough time
>    // has passed since the last time we did it.
> +  presShell->UpdatePaintingSuppressionStatus();

Do we need this caller as well as PresShell::Paint?
Attachment #8752103 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8752103 [details] [diff] [review]
update painting suppression status in painting flow. v1

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

::: layout/base/nsRefreshDriver.cpp
@@ +1808,5 @@
>    }
>  
>    // Recompute approximate frame visibility if it's necessary and enough time
>    // has passed since the last time we did it.
> +  presShell->UpdatePaintingSuppressionStatus();

https://hg.mozilla.org/mozilla-central/annotate/16663eb3dcfa759f25b5e27b101bc79270c156f2/layout/base/nsRefreshDriver.cpp#l1818

I'm not sure the usage for this ScheduleApproximateFrameVisibilityUpdateNow() call.

We check presShell->IsPaintingSuppressed() in this scope.
Hi Matt,
Could you please check the comment 14?
Flags: needinfo?(matt.woodrow)
tnikkel is probably the best person to comment on that caller, but it seems fine.

Maybe we could get rid of the PresShell:Paint one instead? We go through both of these places during normal painting, and it seems possible that the result could be different (although very rare).
Flags: needinfo?(matt.woodrow)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #14)
> Comment on attachment 8752103 [details] [diff] [review]
> update painting suppression status in painting flow. v1
> 
> Review of attachment 8752103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsRefreshDriver.cpp
> @@ +1808,5 @@
> >    }
> >  
> >    // Recompute approximate frame visibility if it's necessary and enough time
> >    // has passed since the last time we did it.
> > +  presShell->UpdatePaintingSuppressionStatus();
> 
> https://hg.mozilla.org/mozilla-central/annotate/
> 16663eb3dcfa759f25b5e27b101bc79270c156f2/layout/base/nsRefreshDriver.
> cpp#l1818
> 
> I'm not sure the usage for this
> ScheduleApproximateFrameVisibilityUpdateNow() call.
> 
> We check presShell->IsPaintingSuppressed() in this scope.

The reason we check IsPaintingSuppressed there is that we call ScheduleApproximateFrameVisibilityUpdateNow when painting is unsuppressed, and it's somewhat useless to do a frame visibility update before painting is unsuppressed.
Comment on attachment 8752103 [details] [diff] [review]
update painting suppression status in painting flow. v1

Good find!


>+   * Update the painting suppression status immediately.
>+   */
>+  virtual void UpdatePaintingSuppressionStatus() = 0;

Maybe call this CheckIfTimeToUnsuppressPainting?

>   // Recompute approximate frame visibility if it's necessary and enough time
>   // has passed since the last time we did it.
>+  presShell->UpdatePaintingSuppressionStatus();

If we're going to unsuppress painting during this tick I'd prefer we do it as soon as possible in case anyone is making decision based on paint suppression status. So if you could move this as far up ::Tick() as possible that would be good.
Attachment #8752103 - Flags: review?(tnikkel) → review+
rename UpdatePaintingSuppressionStatus() to CheckIfTimeToUnsuppressPainting().
Attachment #8752103 - Attachment is obsolete: true
(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> Comment on attachment 8752103 [details] [diff] [review]
> update painting suppression status in painting flow. v1
>
> >   // Recompute approximate frame visibility if it's necessary and enough time
> >   // has passed since the last time we did it.
> >+  presShell->UpdatePaintingSuppressionStatus();
> 
> If we're going to unsuppress painting during this tick I'd prefer we do it
> as soon as possible in case anyone is making decision based on paint
> suppression status. So if you could move this as far up ::Tick() as possible
> that would be good.

In most case, the painting is triggered by a refresh driver timer.
I think the nsRefreshDriver::Tick() is top enough. There is no suppression checking before nsRefreshDriver::Tick().

The call path are:
mozilla::RefreshDriverTimer::TickRefreshDrivers()
  mozilla::RefreshDriverTimer::TickDriver()
    nsRefreshDriver::Tick()
Flags: needinfo?(bzbarsky)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #21)
> (In reply to Timothy Nikkel (:tnikkel) from comment #18)
> > Comment on attachment 8752103 [details] [diff] [review]
> > update painting suppression status in painting flow. v1
> >
> > >   // Recompute approximate frame visibility if it's necessary and enough time
> > >   // has passed since the last time we did it.
> > >+  presShell->UpdatePaintingSuppressionStatus();
> > 
> > If we're going to unsuppress painting during this tick I'd prefer we do it
> > as soon as possible in case anyone is making decision based on paint
> > suppression status. So if you could move this as far up ::Tick() as possible
> > that would be good.
> 
> In most case, the painting is triggered by a refresh driver timer.
> I think the nsRefreshDriver::Tick() is top enough. There is no suppression
> checking before nsRefreshDriver::Tick().
> 
> The call path are:
> mozilla::RefreshDriverTimer::TickRefreshDrivers()
>   mozilla::RefreshDriverTimer::TickDriver()
>     nsRefreshDriver::Tick()

Yes, nsRefreshDriver::Tick is good. I just meant to put the call to CheckIfTimeToUnsuppressPainting as close to the top of nsRefreshDriver::Tick as possible.
update for comment 22.
move the CheckIfTimeToUnsuppressPainting() call just after GetPresShell().
Attachment #8756751 - Attachment is obsolete: true
To answer the question from comment 12, I don't think it's a problem.
sorry had to back this out for perma failures started with this push like https://treeherder.mozilla.org/logviewer.html#?job_id=28876959&repo=mozilla-inbound
Flags: needinfo?(hshih)
when this landed we had a regression on android (autophone - nexus 4 and nexus 6) and reset the values when the backout took place:
https://treeherder.mozilla.org/perf.html#/alerts?id=1340
(In reply to Joel Maher (:jmaher) from comment #30)
> when this landed we had a regression on android (autophone - nexus 4 and
> nexus 6) and reset the values when the backout took place:
> https://treeherder.mozilla.org/perf.html#/alerts?id=1340

Unsuppressing painting sooner makes us do more work (more painting), but it has the advantage of displaying page contents (instead of an empty background) to the user faster. I think snorp was investigating paint suppression on android a while back.
Hi James,
I have a patch to make the unsuppressing painting more precisely. Before that, we use a timer object to trigger unsuppressing painting. When the main thread is busy, the timer task might not be processed in time.
So the patch try to check the unsuppressing painting status during the rendering. If we see the timeout, make the unsuppressing painting off immediately.

With this patch, we had a performance regression on android
https://treeherder.mozilla.org/perf.html#/alerts?id=1340
Do you have any suggestion for this regression?
Flags: needinfo?(hshih) → needinfo?(snorp)
Yeah, I was looking at this in bug 1150172. Painting blocks the main loop, which is pretty critical during page load. I looked into suppressing paints even longer than we do now.

I would not be surprised if the timer fires late on Android due to this main loop congestion. Maybe try making the paint suppression timeout higher with your patch?
Flags: needinfo?(snorp)
I put a try with 500ms timeout value. Wait for the result.
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #36)
> The distance between texts are different between the test and ref html.
> http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/mobile/try-
> builds/hshih@mozilla.com-311bdc1951faeddf65e6ba0c21c1f39404148653/try-
> android-api-15/try_ubuntu64_vm_armv7_large_test-plain-reftest-7-bm117-tests1-
> linux64-build228.txt.gz&only_show_unexpected=1

My guess is that unsuppressing painting sooner means that we draw the page between the image is decoded, whereas before your patch the first time we drew the page the image is already decoded. An android in the past I've seen the pixels of an image be drawn slightly differently depending on if we are drawing just the image, or the whole page. To see if this is the case you could dump what rects we paint in this test before and after your patch.

> The gif img's color is different between test and ref html.
> http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/mobile/try-
> builds/hshih@mozilla.com-311bdc1951faeddf65e6ba0c21c1f39404148653/try-
> android-api-15-debug/try_ubuntu64_vm_armv7_large-debug_test-plain-reftest-18-
> bm52-tests1-linux64-build115.txt.gz&only_show_unexpected=1

This test seems to use an animated gif? If I load the text in my browser then it alternates between red and green forever. Seems like the test is broken.
(In reply to Timothy Nikkel (:tnikkel) from comment #37)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #36)
> > The distance between texts are different between the test and ref html.
> > http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> > reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/mobile/try-
> > builds/hshih@mozilla.com-311bdc1951faeddf65e6ba0c21c1f39404148653/try-
> > android-api-15/try_ubuntu64_vm_armv7_large_test-plain-reftest-7-bm117-tests1-
> > linux64-build228.txt.gz&only_show_unexpected=1
> 
> My guess is that unsuppressing painting sooner means that we draw the page
> between the image is decoded, whereas before your patch the first time we
> drew the page the image is already decoded. An android in the past I've seen
> the pixels of an image be drawn slightly differently depending on if we are
> drawing just the image, or the whole page. To see if this is the case you
> could dump what rects we paint in this test before and after your patch.

Thanks Timothy.
But this test is for text with "text-overflow:ellipsis", right? It's not an image. When we set the "scrollLeft", maybe there is an animation for that scrollLeft change. The reftest will trigger an "PresShell::RenderDocument()" call, I will check if there has SuppressionPainting related code inside.
(In reply to Jerry Shih[:jerry] (PTO:6/8-6/12) (UTC+8) from comment #38)
> (In reply to Timothy Nikkel (:tnikkel) from comment #37)
> > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #36)
> > > The distance between texts are different between the test and ref html.
> > > http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> > > reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/mobile/try-
> > > builds/hshih@mozilla.com-311bdc1951faeddf65e6ba0c21c1f39404148653/try-
> > > android-api-15/try_ubuntu64_vm_armv7_large_test-plain-reftest-7-bm117-tests1-
> > > linux64-build228.txt.gz&only_show_unexpected=1
> > 
> > My guess is that unsuppressing painting sooner means that we draw the page
> > between the image is decoded, whereas before your patch the first time we
> > drew the page the image is already decoded. An android in the past I've seen
> > the pixels of an image be drawn slightly differently depending on if we are
> > drawing just the image, or the whole page. To see if this is the case you
> > could dump what rects we paint in this test before and after your patch.
> 
> Thanks Timothy.
> But this test is for text with "text-overflow:ellipsis", right? It's not an
> image. When we set the "scrollLeft", maybe there is an animation for that
> scrollLeft change. The reftest will trigger an "PresShell::RenderDocument()"
> call, I will check if there has SuppressionPainting related code inside.

Hmm, okay. I'd still start with seeing if the painting pattern has changed from "one big paint of the whole page" to more than one paint for smaller rects as a starting point.
See Also: → 180241

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: bignose1007+bugzilla → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.