Open
Bug 1271691
Opened 9 years ago
Updated 2 years ago
Adjust suppressed painting setting for a big doc
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
NEW
People
(Reporter: jerry, Unassigned)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
video/quicktime
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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).
Comment 1•9 years ago
|
||
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?
Reporter | ||
Comment 2•9 years ago
|
||
This test patch skip our painting suppression mechanism. I can see a noticeable improvement for the test doc in comment 0.
Reporter | ||
Comment 3•9 years ago
|
||
(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).
Reporter | ||
Comment 4•9 years ago
|
||
s/can't/can/
Reporter | ||
Comment 5•9 years ago
|
||
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.
Reporter | ||
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
That seems pretty reasonable as a general approach.
Flags: needinfo?(bzbarsky)
Reporter | ||
Updated•9 years ago
|
Comment 8•9 years ago
|
||
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+
Reporter | ||
Comment 9•9 years ago
|
||
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.
Reporter | ||
Comment 10•9 years ago
|
||
Update the comparison video.
With attachment 8752103 [details] [diff] [review], the content is shown faster(about 1 second) than the original one.
Reporter | ||
Comment 11•9 years ago
|
||
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)
Reporter | ||
Comment 12•9 years ago
|
||
Hi Boris,
Is it any problem that we call additional PresShell::UnsuppressPainting() call at nsRefreshDriver::Tick() and PresShell::Paint()?
Flags: needinfo?(bzbarsky)
Comment 13•9 years ago
|
||
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+
Reporter | ||
Comment 14•9 years ago
|
||
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.
Reporter | ||
Comment 15•9 years ago
|
||
Hi Matt,
Could you please check the comment 14?
Flags: needinfo?(matt.woodrow)
Comment 16•9 years ago
|
||
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)
Comment 17•9 years ago
|
||
(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 18•9 years ago
|
||
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+
Reporter | ||
Comment 19•9 years ago
|
||
rename UpdatePaintingSuppressionStatus() to CheckIfTimeToUnsuppressPainting().
Reporter | ||
Updated•9 years ago
|
Attachment #8752103 -
Attachment is obsolete: true
Reporter | ||
Comment 20•9 years ago
|
||
Reporter | ||
Comment 21•9 years ago
|
||
(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)
Comment 22•9 years ago
|
||
(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.
Reporter | ||
Comment 23•9 years ago
|
||
update for comment 22.
move the CheckIfTimeToUnsuppressPainting() call just after GetPresShell().
Reporter | ||
Updated•9 years ago
|
Attachment #8756751 -
Attachment is obsolete: true
Reporter | ||
Comment 24•9 years ago
|
||
Reporter | ||
Comment 25•9 years ago
|
||
Please land the attachment 8756778 [details] [diff] [review] to m-c.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf05c2551c6
Keywords: checkin-needed
Comment 26•9 years ago
|
||
To answer the question from comment 12, I don't think it's a problem.
Comment 27•9 years ago
|
||
Keywords: checkin-needed
Comment 28•9 years ago
|
||
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)
Comment 29•9 years ago
|
||
Comment 30•9 years ago
|
||
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
Comment 31•9 years ago
|
||
Comment 32•9 years ago
|
||
(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.
Reporter | ||
Comment 33•9 years ago
|
||
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)
Reporter | ||
Comment 35•9 years ago
|
||
I put a try with 500ms timeout value. Wait for the result.
Reporter | ||
Comment 36•9 years ago
|
||
There are some reftests failed at fennec, but I don't know the reason yet.
I'm search for painting suppression checking in gecko(IsPaintingSuppressed() call).
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
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
Comment 37•9 years ago
|
||
(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.
Reporter | ||
Comment 38•8 years ago
|
||
(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.
Comment 39•8 years ago
|
||
(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.
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Comment 40•2 years ago
|
||
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
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•