Closed Bug 142635 Opened 22 years ago Closed 17 years ago

Painting suppression doesn't unsuppress correctly


(Core :: Layout, defect, P2)






(Reporter: jesup, Assigned: jesup)


(Blocks 1 open bug)


(Keywords: perf, Whiteboard: [whitebox])


(1 file)

This is a spinoff of bug 110106, hyatt's "Investigate Win32-only paint
suppression weirdness" bug.  Sam Emrick posted a 1-liner patch that makes it
actually unsuppress when it's supposed to.  (discussed in n.p.m.performance)

Patchto be attached.
Blocks: 110106
Keywords: mozilla1.0, patch, perf
Target Milestone: --- → mozilla1.0
Patch from 110106 as a patch, against branch.  r/sr=?  Looks like a pretty
obvious "oops" patch.
Hmm.  So, the only thing cv->Show() should do is call mWindow->Show(), which
should make the widgets shown.  Then just below we do set mPaintingSuppressed to
false, and then invalidate the root frame.

So I'm guessing:
 * ShowWindow on Windows is causing a paint, perhaps even synchronously, but
that paint doesn't work if |mPaintingSuppressed| is still set.
 * The Invalidate just below is causing a much later paint (thanks to delayed
paint event prioritization on Windows)
 * This patch seems like it might start causing double-paints, which are a
performance hit.

Maybe we should instead be doing the Invalidate before the cv->Show() ?
Has anybody investigated dbaron's thought-proviking questions?

pros/cons: since 'perception' is more important in this situation (win32-only), 
the (one-off) double paint isn't too critical IMO, compared to the uncertainties 
of the pandora box when trying to change the semantics at the last minute.
Of course doing the analysis is better (emrick?) even if it is just to improve 
the understanding. But to improve the 'perception', I will go for the risk of 
the (one-off) double-paint if the analysis is lacking.

To keep it in context, here is the initial n.p.m.performance post:
Sam Emrick wrote:
> I've investigated a problem that Ivan Eisen is seeing/reporting regarding
> instances of very long times for the initial paint of large/long documents.
> On Windows NT, the problem further seen by lockup of gui responsiveness for
> long periods in document load.
> The source of the problem is such. In layout\html\base\src\nsPresShell.cpp
> implements mPaintSuppressionTimer, with intent to supress initial painting
> of large docuements for a while, default 1200 ms. The timer itself is
> working okay, but the handling of the timer timeout is a bit broken. In
> PresShell::UnsuppressAndInvalidate() is a path leading to cv->Show(); The
> Show() leads to a reflow which should perform the paint. However, it
> doesn't because mPaintingSuppressed = PR_FALSE; hasn't been set yet. The
> mPaintingSuppressed = PR_FALSE needs to be set before the call to Show().
> When changed such, then the initial paint delay time is mush more closely
> honored.
> Sam
> PresShell::UnsuppressAndInvalidate() is a path leading to cv->Show(); The
> Show() leads to a reflow which should perform the paint.

This isn't entirely correct (but it is beside the point). Had a look and there 
is no further reflow at that stage. cv->Show(), i.e. DocumentViewerImpl::Show(), 
leads directly to paint (and although I didn't see it, admittedly there might be 
early returns in the paints because they test for mPaintingSuppressed in the 
presShell and bail out -- but in the adjacent chunk of code to cv->Show(), 
paint-suppression is cleared before doing the Invalidate).

> Maybe we should instead be doing the Invalidate before the cv->Show() ?

Seems to lead to the same effect as emrick's patch since Invalidate() will be 
done after enabling paint.

The current code is doing:
/* as dabron noted, perhaps the paint from cv->Show() is synchronous and might 
fire before the next statement */
  mPaintingSuppressed = PR_FALSE;

emrick's change is to do:
  mPaintingSuppressed = PR_FALSE;

and inverting will just do:
  mPaintingSuppressed = PR_FALSE;

(BTW, what's the rationale of further delaying the paint over there, i.e.,
Changing Priority to P2
Priority: -- → P3
Priority: P3 → P2
Targeting this one 1.1Beta 
Keywords: mozilla1.0mozilla1.1
Target Milestone: mozilla1.0 → mozilla1.1beta
Keywords: mozilla1.2
Moving TM to some more realistic target.
Target Milestone: mozilla1.1beta → mozilla1.3alpha
Whiteboard: [whitebox]
Can we get this in for 1.5alpha ... or even sooner?
Blocks: 203448
Is this still needed with Cairo?

And how do patches get noticed, by a keyword?
We don't have a testcase here. I suggest we close this as RESOLVED INCOMPLETE.
Closed: 17 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.