Open Bug 1150172 Opened 9 years ago Updated 2 years ago

Investigate smarter paint suppression

Categories

(Core :: Layout, defect)

defect

Tracking

()

People

(Reporter: snorp, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 8 obsolete files)

1.17 MB, text/plain
Details
1.42 MB, text/plain
Details
918 bytes, patch
tnikkel
: review+
Details | Diff | Splinter Review
13.97 KB, patch
roc
: review-
Details | Diff | Splinter Review
864 bytes, patch
smaug
: review-
Details | Diff | Splinter Review
In the attached profile, taken while loading nytimes.com, you can see we have three paints that occur before the page is "done". In all three cases, there is only one compositor pass between the paints, so the result is only on the screen for a short time. We spend about 500ms cumulatively on these wasted paints, and considering we spend 1.7s total during the page load, this is pretty bad. There are also compounding affects from blocking the main loop for that amount of time and causing additional work for the compositor, etc.

We should be smarter about suppressing these paints if possible.
The patch is clearly simplistic, but it does seem to get the job done on this test at least. The profile has no spurious paints, and the load time dropped quite a bit.
In both profiles, you can see we do one more paint after the load is considered finished. With the patch, however, it's the only one we do, yielding a 500ms page load improvement.

I think something along these lines is on the right track. Maybe we should always allow the very first paint, however.
Comment on attachment 8586996 [details] [diff] [review]
suppress painting the entire time while loading

For desktop at least I think this will end up making the browser feel a ton slower. I was actually thinking we should reduce the time that we paint suppress (at least for regular desktop scenarios).
(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 8586996 [details] [diff] [review]
> suppress painting the entire time while loading
> 
> For desktop at least I think this will end up making the browser feel a ton
> slower. I was actually thinking we should reduce the time that we paint
> suppress (at least for regular desktop scenarios).

The patch as it stands can definitely make things feel slower. I think always allowing the first paint could help.

On top of that, maybe it would be better to have some rate-limiting of paints rather than suppressing them entirely. That way if you have a page that is just taking forever to load, you'll at least see something relatively soon (like you do now).
Maybe rate limit scheduling of paints in content documents that are still loading?
(In reply to Timothy Nikkel (:tn) from comment #6)
> Maybe rate limit scheduling of paints in content documents that are still
> loading?

This sounds like a good starting point to me.

It may not necessarily affect our decisions in this area in the short term, but I think it's worth considering that in the long term, there may be some significant perceived performance wins from keeping at least layout reasonably up-to-date. We should always be prioritizing work that contributes to the rendering of content within the displayport, and doing that requires that we have an accurate notion of which content *is* within the displayport.
Attachment #8589411 - Flags: review?(tnikkel)
If I'm reading these graphs right, this is a 200ms win on autophone's time to throbber stop test.
Assignee: nobody → blassey.bugs
Comment on attachment 8589411 [details] [diff] [review]
paint_limiting.patch

This seems to only limit synth mouse moves? Not painting? If this is a 200ms win then that is indeed notable and we should look harder at reducing synthmouse moves too.
Note that if we're keying off "loading" to control painting rate that some pages never stop "loading" or don't stop for a Long Time, so a binary switch may be problematic; we may need something more adaptive.  The problem with a more adaptive solution is a purely time-based decision may unlimit (or reduce the limitation) before anything really loads.  So time alone may not be good either; perhaps time since <something> - primary page load?  Time since first (significant?) paint?
(In reply to Timothy Nikkel (:tn) from comment #10)
> Comment on attachment 8589411 [details] [diff] [review]
> paint_limiting.patch
> 
> This seems to only limit synth mouse moves? Not painting? If this is a 200ms
> win then that is indeed notable and we should look harder at reducing
> synthmouse moves too.

limiting synth mouse moves is how paint supression works, which is what I used as a model for paint limiting. Do you have any other suggestions?
Paint suppression should be doing more than just affect synthmouse moves. Looking at "PaintingSuppressed" occurrences it should at least: show the previous page while a new loading page has suppressed painting, stop generating invalidations for dynamic rendering changes, make our display list construct items only for backgrounds if we do end up painting.
Comment on attachment 8589411 [details] [diff] [review]
paint_limiting.patch

This patch, as far as I can tell, only has an effect on dispatching synth mouse moves. If this is really your goal then you should change the name (un/limitpainting). Otherwise I'm confused.
Flags: needinfo?(blassey.bugs)
I'm fine with a name change. I'd also be interested if this addresses what snorp was seeing in his profiles or if we need to do more here.
Flags: needinfo?(blassey.bugs) → needinfo?(snorp)
(In reply to Timothy Nikkel (:tn) from comment #6)
> Maybe rate limit scheduling of paints in content documents that are still
> loading?

I agree that this might be the way to go. I experimented with a bunch of stuff the week before last (was on vacation last week), and it seems to be pretty hard to know exactly when we can do a paint that is going to be a "good" one that can stay on the screen for a while. Putting some kind of rate limit in may be the best we can do.

The way to get the fastest end-to-end load time is to do zero paints while we're still loading, but cnn.com for example doesn't display anything for almost 30s if we do that. I think maybe we just extend the initial paint delay to a much longer value; something like 3-5s? Maybe device-dependent? If we hit that, go ahead and allow rate-limited paints while we're still loading. Initially I thought about allowing the first paint immediately once we can determine it's going to be something decent -- meaning all style and script have executed (but not necessarily for child docs). Then allowing rate-limited paints while loading. As mentioned above, though, I am not sure it's very easy right now to determine a good state for allowing the first "real" paint. Timothy do you know?
Flags: needinfo?(snorp)
NI for last sentence in comment #16
Flags: needinfo?(tnikkel)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #16)
> As mentioned above, though, I am not sure
> it's very easy right now to determine a good state for allowing the first
> "real" paint. Timothy do you know?

I don't have any better ideas sorry.

Was most of the extra time in your profile coming from the synth mouse moves? I'm still curious about this.
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #18)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #16)
> > As mentioned above, though, I am not sure
> > it's very easy right now to determine a good state for allowing the first
> > "real" paint. Timothy do you know?
> 
> I don't have any better ideas sorry.
> 
> Was most of the extra time in your profile coming from the synth mouse
> moves? I'm still curious about this.

No, most of the time in the profile is from painting. I don't know anything about synth mouse moves. Maybe those could cause the invalidations that led to the painting?
NIing roc & bz because of their long exposure to these issues. 

The decision about when-to-paint vs loadtime is an old, old, recurring tradeoff.  Painting (and associated re-layout) take real time on complex sites, and sites are of highly varying utility when partly loaded, so there's no clear "right" solution.

Realize I'm out-of-touch with the details of our current implementation, and that much of what I'm going to say is obvious - but I think it would be good to detail all the major factors, so any solution (and the current state) can be judged on all the positive and negative impacts (and there will be both, especially across different use-cases).

I won't recap all the ways it is or can be done, but instead will list some goals/targets (in no order, and many of these conflict in small or large ways):

*) Load pages as fast as possible
*) Have pages ready for user viewing and interaction (even if not fully loaded) as fast as possible
*) Avoid major visual glitches during incremental load/paint (can be annoying, and if interacting with it can move links out from under the mouse)
*) Work well on low-speed connections (including dialup(!) and 3G and bad-WiFi)
*) Work well on high-speed connections
*) Work well on low-end computers/tablets/phones
*) Work well on high-end computers/tablets/phones
*) Works well on simple sites (sample list?)  Google?  Yahoo??
*) Works well on complex sites with lots of JS and dynamic content (sample list?  CNN, Huffingtonpost, Amazon, Facebook, etc)

More?  (I presume there are) 

Likely a "good" solution will be somewhat adaptive to the site, network speed, CPU speed (idle time indicates we have time that could be used for rendering), etc.

Some of these tradeoffs will interact with other parts of the system, like when CSS and JS includes are pulled and parsed, what the necko code does with prioritizing image and JS/CSS loads, etc.  Making changes here may affect (and be affected by) the tradeoffs made in these other systems.
Flags: needinfo?(roc)
Flags: needinfo?(bzbarsky)
So one thing we may want to do but don't really do now is to do an early layout/paint at whatever point (maybe even when we do now?), but then throttle the refresh driver for the page for another few hundred ms or until onload, whichever happens first.  That can lead to some weird effects with animations during those few hundred ms, I guess..
Flags: needinfo?(bzbarsky)
One interesting thing I've noticed is that even if the root PresShell for the content is suppressing painting, we still spend a lot of time painting browser.xul. I think Fennec might be doing something stupid to cause that.
I thought Fennec didn't even have a XUL document to paint!

snorp, can you tell/show what actually changes during those three paints of nytimes.com? It would be good to have a consensus on which frames we ideally would have rendered and which ones we would have skipped...
Flags: needinfo?(roc)
Comment on attachment 8589411 [details] [diff] [review]
paint_limiting.patch

Clearing review request.

We need a name change and/or understand how/if this is improving page load speed.
Attachment #8589411 - Flags: review?(tnikkel)
I'm working on this again. Kind of a tricky problem. Here's what I know:

Painting during page load, at least on Android, does have a pretty negative effect on page load performance. With the nytimes.com that we use on eideticker, page load (uncached) takes about 2.5s on a Nexus 10. If I don't paint at all during the page load, that goes down to 1.5s.

Suppressing painting during the entire page load is not what we we want to do. Some pages just download an endless amount of crap.

We currently suppress paint in PresShell for the first 250ms of a load. I don't see any reason we should be doing this anymore. It's too short to really prevent much painting during the load, and too long in cases where the first paint could be earlier (and therefore show something useful).

We cannot use the paint suppression method currently used in PresShell to throttle the paint rate during page load. If you allow a paint, then start disallowing for some time, the page content gets replaced with whatever is underneath. In practice, this is either blank or the content of the previous page. No bueno. We may need to come up with a way to allow paints for the top-level PresShell (which is the xul document), but instead of just skipping child PresShells that are suppressed, we use whatever they painted last time. Not entirely sure that's possible ATM.

Since Fennec doesn't really have any chrome (it simply holds a deck of browser elements), we could have a hack that just throttles paints in the refresh driver if any document in the tree has a loading status. I'm going to explore this one today, since it should be relatively easy.
This patch puts the refresh driver into throttled mode if it finds that any
document it's about to paint is not complete. It tries to allow the very first paint,
but that part may need some more work. This definitely makes pages load faster, while
at the same time allowing *some* painting. Another thing I think we'll need to do is
disable the throttling when there is any user input. If you try to scroll the page
while it's throttled, the damaged region won't be painted for at least a second, probably longer.
Attachment #8586996 - Attachment is obsolete: true
Attachment #8623056 - Flags: feedback?(roc)
Attachment #8623056 - Flags: feedback?(matt.woodrow)
Comment on attachment 8623056 [details] [diff] [review]
Throttle refresh driver when any document in subtree is loading

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

Seems to me this is likely to produce "no significant painting" on a lot of pages, when nothing interesting has loaded before we do the first paint.

We do definitely need to paint once after each input event.
Attachment #8623056 - Flags: feedback?(roc) → feedback-
I think ideally we'd paint at the following times during load:
1) When "a significant amount" of content has been loaded. I don't have a good heuristic for this unfortunately. We need to avoid flash-of-unstyled-content of course. Maybe the first occurrence of "(all pending stylesheets loaded OR timeout has elapsed) AND (at least 10 presentation elements under <body> OR HTML document finished)" ... though that's no good if the only interesting content is images. This is hard.
2) When a paint is pending and we're otherwise idle.
3) Immediately after an input event.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> I think ideally we'd paint at the following times during load:
> 1) When "a significant amount" of content has been loaded. I don't have a
> good heuristic for this unfortunately. We need to avoid
> flash-of-unstyled-content of course. Maybe the first occurrence of "(all
> pending stylesheets loaded OR timeout has elapsed) AND (at least 10
> presentation elements under <body> OR HTML document finished)" ... though
> that's no good if the only interesting content is images. This is hard.

I agree, this is really hard. I am not sure we need this part to be perfect, though, in order to get some major performance improvements.

> 2) When a paint is pending and we're otherwise idle.

I'm not sure I agree here. What does idle mean? It seems like could easily end up jamming the main loop like we are now, blocking resource loads from being delivered.

> 3) Immediately after an input event.

Yeah, definitely need this. For 1.0, I was thinking we'd just unthrottle completely if there is input during the load.

What do you think about the general idea of throttling in the refresh driver? Is that the right place to be doing this? Should we run the driver at full-speed, but just skip painting?
Flags: needinfo?(roc)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #29)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> > I think ideally we'd paint at the following times during load:
> > 1) When "a significant amount" of content has been loaded. I don't have a
> > good heuristic for this unfortunately. We need to avoid
> > flash-of-unstyled-content of course. Maybe the first occurrence of "(all
> > pending stylesheets loaded OR timeout has elapsed) AND (at least 10
> > presentation elements under <body> OR HTML document finished)" ... though
> > that's no good if the only interesting content is images. This is hard.
> 
> I agree, this is really hard. I am not sure we need this part to be perfect,
> though, in order to get some major performance improvements.

Sure, but it need to avoid killing *apparent* performance too.

Hmm, what if we paint at the first moment when we've opened the body element and all pending style sheets have loaded? We really shouldn't paint before that, so that's about as aggressive as we can be about painting early.

> > 2) When a paint is pending and we're otherwise idle.
> 
> I'm not sure I agree here. What does idle mean? It seems like could easily
> end up jamming the main loop like we are now, blocking resource loads from
> being delivered.

Fair comment.

> > 3) Immediately after an input event.
> 
> Yeah, definitely need this. For 1.0, I was thinking we'd just unthrottle
> completely if there is input during the load.

Yeah, that's probably good.

> What do you think about the general idea of throttling in the refresh
> driver? Is that the right place to be doing this? Should we run the driver
> at full-speed, but just skip painting?

We could, but that's not as good as throttling it properly.
Flags: needinfo?(roc)
No longer blocks: 1164539
Later patches will try to avoid painting a loading document until this state is reached.
Attachment #8627340 - Flags: review?(roc)
Comment on attachment 8627340 [details] [diff] [review]
Add nsIDocument::READYSTATE_STYLED, which means all stylesheets have loaded

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

I'm not the right reviewer here.
Attachment #8627340 - Flags: review?(roc) → review?(bzbarsky)
Comment on attachment 8627340 [details] [diff] [review]
Add nsIDocument::READYSTATE_STYLED, which means all stylesheets have loaded

Are you sure the patch here doesn't leak the world?  Or does CC manage to save us there, even though we're not holding a strong ref cycle document -> loader -> document?

In any case, simply setting READYSTATE_STYLED once StyleSheetLoaded and no pending loads seems wrong; there might be more sheet loads coming up, no?  We should only make that transition if we're currently in the READYSTATE_INTERACTIVE state, I would think...

What is the point of mDOMStyled/mDOMStyledSet on nsDOMNavigationTiming?  They seem to be write-only.
Attachment #8627340 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8627340 [details] [diff] [review]
> Add nsIDocument::READYSTATE_STYLED, which means all stylesheets have loaded
> 
> Are you sure the patch here doesn't leak the world?  Or does CC manage to
> save us there, even though we're not holding a strong ref cycle document ->
> loader -> document?

You mean because the nsDocument is now the observer for the style sheet loads? The loader only holds a ref to the observer while the load is occurring, so I don't think there is a real cycle.

> 
> In any case, simply setting READYSTATE_STYLED once StyleSheetLoaded and no
> pending loads seems wrong; there might be more sheet loads coming up, no? 
> We should only make that transition if we're currently in the
> READYSTATE_INTERACTIVE state, I would think...

Yup, that sounds reasonable.

> 
> What is the point of mDOMStyled/mDOMStyledSet on nsDOMNavigationTiming? 
> They seem to be write-only.

I mostly just followed the pattern that already exists there. I did miss adding an accessor for mDOMStyled, though. mDOMStyledSet is read in NotifyDOMStyled() to prevent overwriting after it was previously set.
> You mean because the nsDocument is now the observer for the style sheet loads? 

No, I'm talking about the AddObserver call in nsDocument::Init.

> I mostly just followed the pattern that already exists there.

That pattern is there because we expose those other values to script via timestamps on the performance timing interface, no?  This new value, as far as I can tell, is not meant to be exposed.

> I did miss adding an accessor for mDOMStyled, though

What calls this accessor?
Flags: needinfo?(snorp)
(In reply to Boris Zbarsky [:bz] from comment #36)
> > You mean because the nsDocument is now the observer for the style sheet loads? 
> 
> No, I'm talking about the AddObserver call in nsDocument::Init.

Ah, right. Yup, that seems bad. I think we should unhook it in Destroy()

> 
> > I mostly just followed the pattern that already exists there.
> 
> That pattern is there because we expose those other values to script via
> timestamps on the performance timing interface, no?  This new value, as far
> as I can tell, is not meant to be exposed.
> 
> > I did miss adding an accessor for mDOMStyled, though
> 
> What calls this accessor?

Nothing now, but I thought maybe it would be useful later. I removed it for now.
Flags: needinfo?(snorp)
Attachment #8627687 - Attachment is obsolete: true
Attachment #8627687 - Flags: review?(bzbarsky)
Attachment #8627809 - Flags: review?(bzbarsky)
>I think we should unhook it in Destroy()

Note that Destroy is not actually guaranteed to be called for all documents until ~nsDocument runs, last I checked.

Again, the real test here is whether there are actual leaks.  I assume you've done a try run for this stuff?
(In reply to Boris Zbarsky [:bz] from comment #39)
> >I think we should unhook it in Destroy()
> 
> Note that Destroy is not actually guaranteed to be called for all documents
> until ~nsDocument runs, last I checked.

It's called in nsDocumentViewer::Destroy right before the reference is cleared. Maybe that isn't always called for all documents?

> 
> Again, the real test here is whether there are actual leaks.  I assume
> you've done a try run for this stuff?

I did, but it was just for perf testing. I'll do a full one.
> Maybe that isn't always called for all documents?

There are lots of documents without a document viewer.  XHR response documents, XBL documents, etc, etc, etc.

Make sure the try run you do has debug builds in it.
Comment on attachment 8627809 [details] [diff] [review]
Add nsIDocument::READYSTATE_STYLED, which means all stylesheets have loaded

>+++ b/accessible/base/Logging.cpp
>+    case nsIDocument::READYSTATE_STYLED:

Shouldn't this come after the "interactive" case, since conceptually STYLED comes after INTERACTIVE?

>+++ b/dom/base/nsDocument.cpp
>@@ -1742,16 +1742,17 @@ nsDocument::~nsDocument()
>+    mCSSLoader->RemoveObserver(this);

Seems pointless, since if mCSSLoader is still holding on to us as an observer we can't reach this function at all.

>XULDocument::StyleSheetLoaded(CSSStyleSheet* aSheet,

So this is bad.  Now we'll have the XULDocument both as as generic observe and as a per-load observer for its sheets, which means its mPendingSheets count will get out of whack (doe to decrementing twice per sheet load), no?

We should probably just use a separate object as the "watch everything" observer after all, or use a separate object to observe the XUL sheet loads or something.
Attachment #8627809 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #42)
> Comment on attachment 8627809 [details] [diff] [review]
> Add nsIDocument::READYSTATE_STYLED, which means all stylesheets have loaded
> 
> >+++ b/accessible/base/Logging.cpp
> >+    case nsIDocument::READYSTATE_STYLED:
> 
> Shouldn't this come after the "interactive" case, since conceptually STYLED
> comes after INTERACTIVE?

Probably so. Fixed.

> 
> >+++ b/dom/base/nsDocument.cpp
> >@@ -1742,16 +1742,17 @@ nsDocument::~nsDocument()
> >+    mCSSLoader->RemoveObserver(this);
> 
> Seems pointless, since if mCSSLoader is still holding on to us as an
> observer we can't reach this function at all.

Indeed, removed.

> 
> >XULDocument::StyleSheetLoaded(CSSStyleSheet* aSheet,
> 
> So this is bad.  Now we'll have the XULDocument both as as generic observe
> and as a per-load observer for its sheets, which means its mPendingSheets
> count will get out of whack (doe to decrementing twice per sheet load), no?
> 
> We should probably just use a separate object as the "watch everything"
> observer after all, or use a separate object to observe the XUL sheet loads
> or something.

Actually, do we need the per-sheet observer at all? We can probably just restore the noop one (or remove the requirement to have an observer at all) and rely on the global one only.
No, you can't, because sheet loads can happen for reasons other than the document starting them (e.g. @import).
Alright, we now just use a separate object for observing stylesheet loads, which
avoids cycles and does not require changes in XULDocument.
Attachment #8627809 - Attachment is obsolete: true
Attachment #8628296 - Flags: review?(bzbarsky)
Comment on attachment 8628296 [details] [diff] [review]
Add nsIDocument::READYSTATE_STYLED, which means all stylesheets have loaded

>+class DocumentStyleSheetObserver final : public nsICSSLoaderObserver {

Why does this need to live in the header (as opposed to just being forward-declared in the header)?

>+  // The document owns us, no strong ref required

Why can't we outlive the document?

>+  nsRefPtr<mozilla::css::Loader> mLoader;

This creates a cycle observer -> loader -> observer, no?  Doesn't this leak the world?

What I think you probably want here is to hold a weak ref to the document, have ~nsDocument null that ref out, and not hold a ref to the loader at all, getting it from the document instead.

>+  mLoader->AddObserver(this);

Passing a 0-refcount XPCOM object around to things that will refcount it is not that great an idea.  Better to AddObserver after the constructor returns and we're being kept alive properly by a refpointer.

>+  mLoader->RemoveObserver(this);

Again, doing this in a destructor makes no sense; if we're being destroyed we've already been removed as an observer.
Attachment #8628296 - Flags: review?(bzbarsky) → review-
Attachment #8628296 - Attachment is obsolete: true
Since error pages are loaded with LOAD_BACKGROUND, they don't call onload
or do any other progress reporting (!). Mark these as complete when the document
has finished.
Attachment #8648261 - Flags: review?(khuey)
Comment on attachment 8648259 [details] [diff] [review]
Clear the first paint appropriately on subdocument presshells

You probably only want to clear the first paint flag if:
1) we are actually painting to the screen (and not building a display list for hit testing, or doing a drawwindow)
2) the subdocument isn't paint suppressed

So check aBuilder->IsPaintingToWindow(), subdocRootFrame, and presShell->IsPaintingSuppressed().
Attachment #8648259 - Flags: review?(tnikkel) → review+
Comment on attachment 8648261 [details] [diff] [review]
Set READYSTATE_COMPLETE for error pages

I'm not comfortable reviewing this, sorry.  Try bz or smaug?
Attachment #8648261 - Flags: review?(khuey)
This version unthrottles whenever there is any kind of input, though that's only hooked up for Android here.
Attachment #8589411 - Attachment is obsolete: true
Attachment #8623056 - Attachment is obsolete: true
Attachment #8623056 - Flags: feedback?(matt.woodrow)
Attachment #8651889 - Flags: review?(roc)
Comment on attachment 8648261 [details] [diff] [review]
Set READYSTATE_COMPLETE for error pages

Any reason why you don't just check mChannel's flags in EndLoad?
The patch doesn't give us quite the right behavior, but given that we're anyway missing load event (!) doesn't matter too much.


About the other patch
I'd prefer if bool varcache was used for layout.pageload-throttling.enabled.
And also for layout.pageload-throttling.interval_ms.
the caches are super simple to use, and reduce extra pref calls.
Attachment #8648261 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (high review load) from comment #52)
> Comment on attachment 8648261 [details] [diff] [review]
> Set READYSTATE_COMPLETE for error pages
> 
> Any reason why you don't just check mChannel's flags in EndLoad?

Uh, nope. New patch coming.

> The patch doesn't give us quite the right behavior, but given that we're
> anyway missing load event (!) doesn't matter too much.
> 

Yeah, figured it's better than nothing.
 
> About the other patch
> I'd prefer if bool varcache was used for layout.pageload-throttling.enabled.
> And also for layout.pageload-throttling.interval_ms.
> the caches are super simple to use, and reduce extra pref calls.

Thanks, I'll do that.
Comment on attachment 8651889 [details] [diff] [review]
Throttle refresh driver when any document in subtree is loading

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

Please split this into the PresContext freeze/thaw refresh driver part, and the RefreshDriver throttling part.

This is ultra-aggressive and I worry about third-party subdocuments like ads interfering with decisions on when we paint content. But I guess I can live with the approach...

::: layout/base/nsPresContext.cpp
@@ +1046,5 @@
>  
> +  nsIDocument::ReadyState rs = mDocument->GetReadyStateEnum();
> +  if (rs && rs != nsIDocument::READYSTATE_COMPLETE &&
> +      this != GetDisplayRootPresContext() &&
> +      Preferences::GetBool("layout.pageload-throttling.enabled", false))

Please explicitly compare rs against READYSTATE_UNINITIALIZED instead of just the implicit zero compare here.

I think it would be better to have a single function UpdateRefreshDriverForReadyState that encapsulates the checks here and in NotifyReadyStateChanged and freezes or thaws as necessary. You will need a new flag on nsPresContext to track if we've frozen the refresh driver for the document readystate, but I think that makes this more robust.

Please add your new prefs to all.js with documentation.
Attachment #8651889 - Flags: review?(roc) → review-
Comment on attachment 8652600 [details] [diff] [review]
Set READYSTATE_COMPLETE for error pages

You need to null check mChannel and add some comment why we're doing this.
Attachment #8652600 - Flags: review?(bugs) → review+
Comment on attachment 8652600 [details] [diff] [review]
Set READYSTATE_COMPLETE for error pages

I realized we don't want to do this this way, since we do end up 
executing nsDocumentViewer::LoadComplete for error loads too.
So, move SetReadyStateInternal call there to be called for error loads too.
Attachment #8652600 - Flags: review+ → review-
(move or have it called also in error load cases)
Spoke to :snorp and he said that he's not working on this at the moment; but mentioned that he may be able to break some of the valuable paint throttling work out into separate bug. NI'ing :snorp to give more detail.
Flags: needinfo?(snorp)
I still haven't really had time to get back to this. I think what I would look into is removing the current concept of paint suppression in nsPresShell, and instead just don't paint in the refresh driver at all if the doc is not sufficiently "ready". For B2G apps, ready could be mean completely loaded or something.
Flags: needinfo?(snorp)
Assignee: lassey → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.