Closed Bug 243726 Opened 20 years ago Closed 20 years ago

Lots of time spent on view-twiddling in DHTML testcase

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: roc)

References

(Depends on 2 open bugs,
URL
)

Details

(Keywords: meta, perf)

Attachments

(3 files, 3 obsolete files)

Profile data
(deleted), application/zip
Details
v3
(deleted), patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
remove childCovers
(deleted), patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
The testcase is actually test number 6 at this page.  Its takes 50 absolutely
positioned divs with z-indices and moves them around.  I see about 12% of total
time spent under nsViewManager::UpdateView(nsIView*, nsRect const&, unsigned). 
The callers are evenly split between:

  nsViewManager::UpdateView(nsIView*, unsigned)
  nsIFrame::Invalidate(nsRect const&, int) const
  nsViewManager::MoveViewTo(nsIView*, int, int)

The UpdateView() calls all come through UpdateViewsForTree, which is called from
the ApplyRenderingChangeToTree() calls made by style reresolution.

Now the Invalidate() calls all come from inside reflow.

MoveViewTo comes from nsContainerFrame::PositionFrameView (called from
nsAbsoluteContainingBlock::ReflowAbsoluteFrame).

So it looks to me like we're calling UpdateView 3 times for each view
involved.... (possibly with different rects, I guess).

The 15914 hits under UpdateView are split as follows:

 14614 nsViewManager::UpdateWidgetArea(nsView*, nsRect const&, nsView*)
   605 nsView::GetClippedRect()
   336 nsViewManager::GetRectVisibility(nsIView*, nsRect const&, unsigned short,
                                        nsRectVisibility*)
    38 nsViewManager::ComputeViewOffset(nsView const*)
    32 nsBaseWidget::GetChildren()
    22 nsBaseWidget::Enumerator::CurrentItem(nsISupports**)
    21 nsBaseWidget::Enumerator::First()
    21 ApplyClipRect(nsView const*, nsRect*, int)
    20 nsCOMPtr_base::begin_assignment()
    19 nsRect::IntersectRect(nsRect const&, nsRect const&)
    17 nsViewManager::GetWidgetForView(nsIView*, nsIWidget**)
    16 nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&)
    13 nsView::GetViewFor(nsIWidget*)
    10 nsViewManager::GetVisibleRect(nsRect&)

The GetClippedRect() time is almost all spent in (not under, but in)
ApplyClipRect().

The UpdateWidgetArea() time is split as follows:

         7403 nsWindow::Invalidate(nsRect const&, int)
         3072 nsViewManager::AddRectToDirtyRegion(nsView*, nsRect const&) const
          904 nsBaseWidget::GetChildren()
          771 nsCOMPtr_base::~nsCOMPtr_base()
          445 nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&)
          298 nsView::GetViewFor(nsIWidget*)
          201 nsBaseWidget::Enumerator::CurrentItem(nsISupports**)
          159 nsViewManager::GetWidgetForView(nsIView*, nsIWidget**)
          120 nsBaseWidget::Enumerator::Next()
          107 nsViewManager::ViewToWidget(nsView*, nsView*, nsRect&) const
           94 nsRect::IntersectRect(nsRect const&, nsRect const&)
           72 nsWindow::AddRef()
           63 nsWindow::Release()

deCOM of nsIWidget::GetChildren (bug 243724) and nsIViewManager::ViewToWidget
(bug 243723) will help here a tad.

AddRectToDirtyRegion is spending most of its time in nsRegionGTK::Union, which
is just calling gdk_region_union_with_rect (which spends its time in malloc,
XUnionRectWithRegion, etc).

nsWindow::Invalidate is also spending its time in nsRegionGTK::Union.

Total time spent in nsRegionGTK::Union is 8% of the time for this testcase. 
That's 2/3 of the time spent under viewmanager code!  So anything we can do here
would help.  With any luck, bug 230170 would reduce the number of calls here,
but we should still look into making this code faster...  Bug 243725 filed on that.
Attached file Profile data (deleted) — 

    
    

    
  
Two macro-level things we can do:
-- currently UpdateView figures out which widgets need to be repainted by the
changes to the view, then records that damage in a region attached to the view
associated with each widget. We could delay this step so that UpdateView always
adds the damage to the that view's region, and the assignment of damage to
widgets is computed when the invalidate event is processed. That would make a
lot more sense, actually.

-- we could do what we talked about a while ago, and add a new change hint for
changes to top/left/bottom/right style where the width (resp. height) is not
auto, and use that change hint to optimize incremental reflow.

    
    

    
  
The first item sounds good.

The second may be hard to do (I'd sorta like to wait till after reflow
refactoring gets done to attempt it).

One more thing I thought of last night -- we could change the hint for these
properties from NS_STYLE_HINT_REFLOW to nsChangeHint_ReflowFrame (and deal with
resulting bugs where reflow doesn't invalidate properly?).  That should help
eliminate the ApplyRenderingChangeToTree part of the UpdateView() calls...

In fact, perhaps we should generally work on converting REFLOW hints to ReflowFrame.

Please spin off bugs on these sub-issues as you see fit!

    
    

    
  
Yes, converting the hints would be great.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        implementation of the first item (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
See if this helps your profile.

The patch seems to work but should get some more testing.

    
    

    
  
That patch helps some (5% improvement wall-clock time, about 17% reduction in
UpdateWidgetArea calls, the UpdateViewsForTree codepath seems to be where most
of the win was; makes sense, given that that uses view updated batching).

This patch is also likely to speed up other users of update batching (editor,
say), right?

One thing I noticed was that the earlier two-textarea test on the same page
didn't paint the first textarea before starting to work on the second one,
unlike without this patch.  I'm guessing it's a matter of us having an extra
event around now, right?  One event to end up calling UpdateWidgetArea() and
then back out to the mainloop to paint.  It looks like the script that does the
second textarea triggers in between.

I also assume that there's not much we can do about the nsWindow stuff past just
not calling into this code as much?  The breakdown for
nsViewManager::UpdateWidgetArea with this patch is:

489   13388 nsViewManager::UpdateWidgetArea(nsView*, nsRegion const&, nsView*)
       9310 nsWindow::Invalidate(nsRect const&, int)
        633 nsBaseWidget::GetChildren()
        587 nsCOMPtr_base::~nsCOMPtr_base()
        343 nsRegion::And(nsRegion const&, nsRect const&)
        306 nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&)

etc, with total time on the testcase being 147931 (so Invalidate() is still 6-7%
or so of total time, looks like?).  Should presshell call
BeginUpdateViewBatch/EndUpdateViewBatch before/after processing reflow commands,
maybe?

    
  
Depends on: 243882

    
    

    
  
Hmm... if I batch view updates around reflow invalidates seem to get lost
(testing in a build with your patch in it, will test without this patch in a
sec).  Some things (created via DHTML) just never end up painting until I select
around them or something....  The DHTML animation we're looking at here becomes
completely invisible.  :(

    
    

    
  
OK, without this patch but _with_ view batching in the presshell while
processing reflow commands, I do see a few minor artifacts (halves of a single
word getting painted in obviously different paint cycles!) but not the problems
I described in comment 8...

Roc, wanna try putting "mViewManager->BeginUpdateViewBatch();" in 
PresShell::ProcessReflowCommands right after we set mIsReflowing to true and
"mViewManager->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);" right before we set
mIsReflowing to false and seeing what's going on?

    
    

    
  
> This patch is also likely to speed up other users of update batching (editor,
> say), right?

Yes.

> Should presshell call BeginUpdateViewBatch/EndUpdateViewBatch before/after
> processing reflow commands, maybe?

I thought we did already.

> One thing I noticed was that the earlier two-textarea test on the same page
> didn't paint the first textarea before starting to work on the second one,
> unlike without this patch.

Not sure why that is.

I'll take a look at your suggestion, but I may not have the cycles to do it soon.

    
    

    
  
> I thought we did already.

I would have thought so too, but we don't.  That seems to be a major factor in
the reflow-triggered invalidates taking so much time....

> Not sure why that is.

I thought about it some more, and I'm pretty sure my analysis of that part is
right.  I just looked through your patch, and I see nothing obviously wrong, so
I'm not sure what's up with its interaction with view batching.  :(

    
    

    
  
Well, _something_ fishy is up.  aView->HasNonEmptyDirtyRegion() is always null
in ProcessPendingUpdates() on the testcase that doesn't paint at all.  I checked
that HasNonEmptyDirtyRegion() does return 1 when we Or() into the region... the
problem is that ProcessPendingUpdates seems to never even be called on the view
(and I mean _the_ view -- there is only one in this case) that
UpdateWidgetArea() was working with when it stashed away the region info!

Could this be a problem with subframes?   It looks like
nsViewManager::UpdateView() does this walk up to realRoot (which would be the
root of a _parent_ viewmanager, no?) in nsViewManager::UpdateView (the version
that takes a rect).  So UpdateWidgetArea would get called on the root view of
the root viewmanager, while ProcessPendingUpdates would only get called on the
root view of the subframe's viewmanager (since the subframe's viewmanager is
what's getting the begin/end batches and since the subframe's viewmanager is
what's setting the "have pending updates" flags).  Frankly, I'm surprised this
hasn't bitten us before....

    
    

    
  
Ah yes. Really we want the paint event to always be handled by the root of the
view managers.

    
    

    
  
and begin/end batch should be propagated to the root of the view managers, too.

    
  
Depends on: 244238

    
  
Blocks: 21762

    
  
Blocks: 249546

    
  
Depends on: 244290

    
  
Blocks: 229391

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated to trunk (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
It seems to just work.

    
    

    
  
But I'm concerned about what happens when the dirty region gets very complex. It
looks like our performance could collapse because each region operation becomes
O(N) for some large N. (E.g., if there are lots of very small non-overlapping
elements moving around on the page.) Perhaps what we need here is some kind of
"loose" region that is allowed to make itself bigger (up to its bounding box) if
necessary to keep the complexity down. What do you guys think?
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
oops
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Don't we already have the same performance issues with native regions?  Or do
those already optimize themselves somehow?  I certainly recall native regions
showing up as a heavy hit in cases when we have lots of little invalidate rects
flying about.

I do think that the loose region idea is a good one.  If we did that, we may
even be able to partially undo the changes we made in bug 253001.

    
    

    
  
Do you want nsRegion to internally decide when it becomes too complex and
automatically simplify itself to a bounding box?

If yes then we can add one extra parameter to constructor (or separate setter) 
which will set a maximal complexity of region. By default it will be unlimited,
but some callers can explicitly specify it.
Something like:

  nsRegion (PRUint32 MaxComplexity = PR_UINT32_MAX);

  nsRegion mDirtyRegion (200);

    
    

    
  
I was thinking that we could have a new method nsRegion::Simplify(PRInt32
aMaxRects) which reduces the complexity if necessary. It would be relatively
easy to put in calls to Simplify where we may need them.

We don't necessarily need to give up and blow out to the bounding box. If the
region exceeds the rect limit, Simplify could merge adjacent rects in the rect
list until the rect list reaches the limit. If we were being really cool, we
would look for adjacent pairs of rects where the difference between the area of
the union and the sum of the areas of the two rects is smallest (i.e., merging
would add the minimal unnecessary space). Make sense? With careful
implementation (using a decent priority queue) I believe this simplification
process could be implemented in O(N log N) where N is the current number of
rects. When the current number of rects is at most aMaxRects + 1, which it would
be in the cases we care about, then it would be easy to make the simplification
be O(aMaxSize) which is effectively constant.

    
    

    
  
(FWIW I've been riding with this trunk patch for most of today and gave it a
spin on various DHTML pages.  Didn't see any obvious problems.)


    
    

    
  
OK, I applied that patch and ran the testcase in bug 229391 (which has a _lot_
of absolutely positioned divs, hence a very complex region).  Performance
actually suffered quite a bit from this patch (it's twice as slow). 
UpdateWidgetArea() took about 10 times as long as pre-patch (going from about 7%
of the testcase to about 50%, with the main gain of time spent being in
nsRegion::IsEqual, nsRegion::Or, and nsWindow::Invalidate.  In fact,
nsWindow::Invalidate takes just as much time with this patch as
AddRectToDirtyRegion() used to take without... the time being spent largely in
nsRegionGTK::Union, presumably as we union the rects in our nsRegion onto the
GTK region one by one.

So I agree that some sort of region-simplification would need to happen for us
to use this patch... otherwise it'll help in some cases and hurt in others.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        adding region simplification (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This does the simplest thing possible: just expand to the bounding box when the
region gets too complex. Doing something intelligent seemed really tricky, and
it's not obvious yet that we need to.

        Attachment #148660 -
        Attachment is obsolete: true

        Attachment #164055 -
        Attachment is obsolete: true

    
    

    
  
Ok, with the latest patch, still on the testcase in bug 229391, I see the
following effect:

1)  Time in UpdateWidgetArea drops from 24234 to 484 hits.
2)  As a consequence, the time spent in ProcessReflowCommands drops from 45232
    to 19276 hits. 
3)  Time in ProcessPendingUpdates() goes up from 170 to 252 hits (makes sense;
    it does a bit more work now).
4)  Time spent in nsWindow::Update (called via UpdateIdle) goes up from 3850 to
    8287 hits.  Which is a little odd, since on Linux we just update the
    bounding box of the dirty region, no?

So overall, this looks like a win on that testcase....

    
    

    
  
Oh, on testcase 6 at http://www.24fun.com/downloadcenter/benchjs/benchjs.html
(where there are only about 50 objects around, not thousands), this patch seems
to be about 8% improvement in overall time.

So with this patch we're only about 50% slower than Opera 7.  ;)

    
    

    
  
> Time spent in nsWindow::Update (called via UpdateIdle) goes up from 3850 to
> 8287 hits.  Which is a little odd, since on Linux we just update the
> bounding box of the dirty region, no?

nsWindow::Update calls DoPaint which actually paints. The increase is probably
just because we can process more frames so we can paint more. Right?

What's the overall speedup on that testcase, just for curiosity?

> So with this patch we're only about 50% slower than Opera 7.  ;)

That's something to work on, but ultimately I don't give two hoots about Opera 7 :-)

Let's get this baby reviewed and checked in.

    
    

    
  
Comment on attachment 164144 [details] [diff] [review]
adding region simplification

Dainis, I just want your review on the nsRegion code. Boris, if you could r+sr
the rest...

        Attachment #164144 -
        Flags: superreview?(bzbarsky)

        Attachment #164144 -
        Flags: review?(Dainis_Jonitis)

    
    

    
  
(In reply to comment #26)
> nsWindow::Update calls DoPaint which actually paints. The increase is probably
> just because we can process more frames so we can paint more. Right?

Possibly.  I did profile over the entire duration of the animation in both cases...

With the patch, the time taken by BuildRenderingDisplayList() also jumped, so I
do suspect you're right.

> What's the overall speedup on that testcase, just for curiosity?

Without patch:

  Elapsed time: 18'601 ms
  Average per loop: 372.02 ms
  Fastest loop: 298 ms
  Slowest loop: 484 ms

With patch:

  Elapsed time: 17'411 ms
  Average per loop: 348.22 ms
  Fastest loop: 284 ms
  Slowest loop: 461 ms

So somewhere around 7-8%, as expected based on comment 0.  ;)

I'll review this tonight.

    
    

    
  
Comment on attachment 164144 [details] [diff] [review]
adding region simplification

r=Dainis Jonitis on nsRegion changes

        Attachment #164144 -
        Flags: review?(Dainis_Jonitis) → review+

    
    

    
  
Comment on attachment 164144 [details] [diff] [review]
adding region simplification

>Index: view/src/nsScrollPortView.cpp

>+    // Since we keep track of the dirty region as absolute coordintes,

"coordinates"

So the UpdateViewAfterScroll() call was just wrong before?

>Index: view/src/nsView.h

>+  PRBool HasNonEmptyDirtyRegion() {
>+    if (!mDirtyRegion) {
>+      return PR_FALSE;
>+    } else {
>+      return !mDirtyRegion->IsEmpty();
>+    }
>+  }

This may be more readable as:

  return mDirtyRegion && !mDirtyRegion->IsEmpty();

>Index: view/src/nsViewManager.cpp

[comments before UpdateWidgetArea]
> // Returns true if this view's widget(s) completely cover the rectangle
> // The specified rectangle, relative to aWidgetView, is invalidated in every widget child of aWidgetView,
> // plus aWidgetView's own widget
> // If non-null, the aIgnoreWidgetView's widget and its children are not updated.

What rectangle is this talking about?  Does "return" mean "set
*aCoveredRegion"?  Fix these comments, please.

>+  if (aCoveredRegion) {
>+    *aCoveredRegion = PR_FALSE;
>+  }

No reason to do this before we check whether refresh is enabled.  In the
!IsRefreshEnabled() case, we should never be getting called with a non-null
aCoveredRegion.

Maybe move this to after the !IsRefreshEnabled() block and assert
!aCoveredRegion in that block?

>+    nsRegion* dirtyRegion = aWidgetView->DirtyRegion();

That could return null on OOM.	Check for null and return if it is?

The rest looks ok, but I'd like to take one more look once the comments are
fixed to make sure I'm understanding things right.

        Attachment #164144 -
        Flags: superreview?(bzbarsky) → superreview-

    
    

    
  
(In reply to comment #30)
> "coordinates"

:-)

> So the UpdateViewAfterScroll() call was just wrong before?

Actually it doesn't use its aDx and aDy parameters. I didn't notice that I'd
changed them from pixels to twips but I think twips is better if we ever do want
to use them...

> This may be more readable as:
> 
>   return mDirtyRegion && !mDirtyRegion->IsEmpty();

Right

> >Index: view/src/nsViewManager.cpp
> 
> [comments before UpdateWidgetArea]
> What rectangle is this talking about?  Does "return" mean "set
> *aCoveredRegion"?  Fix these comments, please.

Oops yeah

> >+  if (aCoveredRegion) {
> >+    *aCoveredRegion = PR_FALSE;
> >+  }
> 
> No reason to do this before we check whether refresh is enabled.  In the
> !IsRefreshEnabled() case, we should never be getting called with a non-null
> aCoveredRegion.
> Maybe move this to after the !IsRefreshEnabled() block and assert
> !aCoveredRegion in that block?

Right

> >+    nsRegion* dirtyRegion = aWidgetView->DirtyRegion();
> 
> That could return null on OOM.	Check for null and return if it is?

Then I'd have to rename it to GetDirtyRegion. Guess I'll do that.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 (deleted)
        — 
          — Splinter Review
      

    


  
Addresses bz comments

        Attachment #164144 -
        Attachment is obsolete: true

        Attachment #164271 -
        Flags: superreview?(bzbarsky)

        Attachment #164271 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 164271 [details] [diff] [review]
v3

r+sr=bzbarsky

        Attachment #164271 -
        Flags: superreview?(bzbarsky)

        Attachment #164271 -
        Flags: superreview+

        Attachment #164271 -
        Flags: review?(bzbarsky)

        Attachment #164271 -
        Flags: review+

    
    

    
  
Checked in.
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED

    
    

    
  
Either this or bug 265165 hurt Tp and Tdhtml on luna and btek. Not sure which.
I'll try backing out 265165 first

    
    

    
  
Backing out 265165 didn't help so I'm relanding it.

I'm mystified how this hurts Tp and Tdhtml.

    
    

    
  
Here are the Tdhtml results:
Before patch:
  colorfade:     1738   1755    1841,1724,1729,1744,1738
  diagball:      1982   1968    1860,2044,1983,1982,1971
  fadespacing:   2864   2864    2871,2864,2864,2860,2862
  imageslide:    389    384     363,389,389,388,391
  layers1:       6001   5976    5870,6014,6002,5995,6001
  layers2:       248    248     247,249,249,248,246
  layers4:       248    248     247,248,249,246,249
  layers5:       5711   5743    5841,5711,5791,5676,5697
  layers6:       627    628     627,626,629,631,627
  meter:         1630   1680    1851,1615,1615,1630,1687
  movingtext:    1349   1346    1331,1349,1350,1352,1349
  mozilla:       3194   3453    3111,4617,3201,3144,3194
  replaceimages: 3700   3756    3659,3698,4014,3700,3708
  scrolling:     11679  11766   11679,11649,11634,11939,11930
  slidein:       2882   2876    2843,2880,2887,2889,2882
  slidingballs:  3850   3822    3699,3847,3850,3852,3861
  zoom:          613    611     601,614,612,614,613
After patch:
  colorfade:     1728   1751    1840,1721,1728,1743,1721
  diagball:      1972   1972    1877,2014,1972,2027,1972
  fadespacing:   2861   2864    2871,2859,2856,2861,2871
  imageslide:    418    409     361,418,422,425,417
  layers1:       5287   5285    5287,5286,5312,5246,5294
  layers2:       246    246     245,248,245,248,246
  layers4:       247    248     247,247,249,247,251
  layers5:       4723   4732    4826,4712,4775,4723,4623
  layers6:       626    627     626,625,630,629,625
  meter:         1594   1595    1614,1584,1597,1594,1584
  movingtext:    1327   1387    1325,1629,1325,1331,1327
  mozilla:       3277   3266    3225,3269,3277,3278,3279
  replaceimages: 3657   3654    3633,3670,3648,3657,3661
  scrolling:     11930  12211   11566,11930,13698,11909,11951
  slidein:       2873   2866    2837,2873,2875,2871,2875
  slidingballs:  6433   6407    6299,6438,6433,6432,6434
  zoom:          1035   1040    1035,1016,1035,1060,1056

So it looks like we were mostly neutral or small gains across the board, but
with significant gains in layers1 and layers5, and major losses in slidingballs
and zoom.

It looks like the Tp loss was btek only, and I have officially decided to stop
caring about btek Tp numbers, so I'm not worried about that.

Where are the Tdhtml tests stashed?

    
    

    
  
OK, I found them, but how does one run them manually? They seem to need
Tinderbox hooks to actually run.

Is this documented anywhere?

    
    

    
  
OK OK so I hand edited the testcase.

    
    

    
  
On slidingballs paint flashing shows that we're painting the "right" area. No
problems with too much painting.

A jprof profile of reloading slidingballs 20 times doesn't make anything jump
out at me, but this is a debug build. I'll have to make an optimized build.



    
    

    
  
slidingballs doesn't look too different from regular moving-layers testcases,
except that painting it is a lot slower because each layer is a scaled
transparent image. Is it possible that there's some frame-skipping effect here
which means were painting more frames and slowing down?

    
    

    
  
> They seem to need Tinderbox hooks to actually run.

That was a bug in the slidingballs and zoom testcases (I just checked in the
fix)...  Or did you mean the fact that you can't get numbers out of them by
running them manually?

I'm poking at this now; will comment if I find anything.


    
    

    
  
OK, profiling slidingballs before and after the patch shows that the time in
UpdateWidgetArea() went down from 1024 hits to 109 hits.  The time in Update()
went from 10645 hits to 20174.  That accounts for the slowdown.  This matches
what I saw in comment 4.  Again, the time taken by BuildRenderingDisplayList()
is scaled just like the time taken by RenderViews, suggesting that we simply run
the code more often.... not yet sure why that happens.

    
    

    
  
I think I know what's up here.  More in an hour when I'm done with class.

    
    

    
  
Brief summary:  The childCovers optimization is insufficient, because our dirty
region includes the scrollbar.  So we end up painting everything twice -- once
in the viewport widget, and once in the scrollwidget. Before this patch, the
childCovers optimization was applied on a per-rect basis, so most of the rects
would in fact have been covered.

Per discussion with roc, the solution is to subtract out the child areas from
the damage region as we go through kids.

    
    

    
  
New bug? Reopen this bug?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        remove childCovers (deleted)
        — 
          — Splinter Review
      

    


  
This just sums up the area of the child widgets into a region and uses that to
figure out what needs to be invalidated in the parent. Seems to fix the problem
as far as I can tell.

    
    

    
  
Comment on attachment 164668 [details] [diff] [review]
remove childCovers

The toolkit should really be ensuring that it doesn't ask us to paint widget
areas that are behind other widgets. But GTK1 is lame and I guess avoiding the
call to native Invalidate is probably worth something.

        Attachment #164668 -
        Flags: superreview?(bzbarsky)

        Attachment #164668 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 164668 [details] [diff] [review]
remove childCovers

>Index: view/src/nsViewManager.cpp
>+  // Update all child widgets with the damage. In the process,
>+  // accumulate the union of all the child widget areas, or at least
>+  // some subset of that.

We used to bail out if the view of the child widget covered the damage area...
is there a reason for this change?  I'm still not quite clear on what
assumptions we can make about the view and widget rects matching up.

>+    nsRect r;
>+    childWidget->GetBounds(r);
>+    r.ScaleRoundIn(mContext->DevUnitsToAppUnits());

In particular, do we really want to do this if there was no view associated
with the widget?  After all, we didn't invalidate this area in that case... 
Wouldn't it make more sense to put this code where we call UpdateWidgetArea on
the kid and to add the dimensions of the child view (converted to our coords)
to the children region?

    
    

    
  
(In reply to comment #50)
> We used to bail out if the view of the child widget covered the damage area...
> is there a reason for this change?  I'm still not quite clear on what
> assumptions we can make about the view and widget rects matching up.

We'll only be doing this when refresh is disabled. If we're inside
ProcessPendingUpdates, we do UpdateWidgetArea after ProcessPendingUpdates on all
our children. So the views and widgets we look at should be consistent.

> >+    nsRect r;
> >+    childWidget->GetBounds(r);
> >+    r.ScaleRoundIn(mContext->DevUnitsToAppUnits());
> 
> In particular, do we really want to do this if there was no view associated
> with the widget?  After all, we didn't invalidate this area in that case...

If the area is covered by a child widget, there's no point in invalidating it in
the parent. That simply won't change anything visible --- whether or not the
child widget is ours.

> Wouldn't it make more sense to put this code where we call UpdateWidgetArea on
> the kid and to add the dimensions of the child view (converted to our coords)
> to the children region?

I'd rather make sure to get all the child widgets. For example if this is a XUL
window and it has a primary <browser> element that is not in our view manager
tree, we don't want to invalidate under it.

    
    

    
  
(In reply to comment #51)
> If the area is covered by a child widget, there's no point in invalidating it
> in the parent. That simply won't change anything visible --- whether or not
> the child widget is ours.

Ah.  This was the part I was missing.

    
    

    
  
Comment on attachment 164668 [details] [diff] [review]
remove childCovers

>Index: view/src/nsViewManager.cpp
>+    nsRect r;
>+    childWidget->GetBounds(r);
>+    r.ScaleRoundIn(mContext->DevUnitsToAppUnits());
>+    // account for the offset between the view and the widget origins

We're accounting for the scale, not for any particular offset, here.... Since
you handle the offset below, I think this comment just needs to be removed.

>+  // children is relative to aWidgetView's widget origin. Need to make
>+  // it relative to the origin of aWidgetView
>+  children.MoveBy(aWidgetView->GetPosition() - aWidgetView->GetBounds().TopLeft());

Is that the right sign?  That is, if "children" has top left at (0,0) relative
to the widget origin (which matches the bounds topleft), and bounds extend to
the left and above the view's position, then after translation into the view's
coordinate system we would expect it to have negative coordinates, right?  But
GetPosition() - GetBounds().TopLeft() will be positive in this case.

r+sr=bzbarsky with those issues resolved.

        Attachment #164668 -
        Flags: superreview?(bzbarsky)

        Attachment #164668 -
        Flags: superreview+

        Attachment #164668 -
        Flags: review?(bzbarsky)

        Attachment #164668 -
        Flags: review+

    
    

    
  
> But GetPosition() - GetBounds().TopLeft() will be positive in this case.

Yeah. I will fix.

    
  
Blocks: 47742

    
    

    
  
This undid the Tp loss on btek and fixed Tdhtml on luna, and improved it a bit
more, from about 1700 before the this bug fix to about 1650 now. Small wins on
many testcases, no significant losses. Thanks bz!

  colorfade:     1731   1800    2086,1729,1712,1740,1731
  diagball:      1900   1896    1858,1897,1911,1913,1900
  fadespacing:   2858   2837    2864,2804,2859,2858,2799
  imageslide:    392    391     369,401,392,389,402
  layers1:       5099   5153    5107,5390,5086,5084,5099
  layers2:       235    235     233,235,233,237,235
  layers4:       236    236     234,237,239,236,234
  layers5:       4577   4590    4592,4484,4804,4577,4492
  layers6:       589    590     589,590,588,589,592
  meter:         1556   1556    1555,1556,1552,1559,1559
  movingtext:    1319   1314    1314,1323,1322,1293,1319
  mozilla:       3199   3460    3115,3187,3199,4590,3207
  replaceimages: 3718   3712    3688,3719,3719,3718,3714
  scrolling:     11512  11541   11477,11523,11481,11512,11712
  slidein:       2885   2878    2849,2885,2886,2883,2886
  slidingballs:  3657   3647    3600,3657,3662,3664,3651
  zoom:          615    616     600,633,615,618,615


    
    

    
  
even money that this regressed camino scrolling speed! yay!

    
    

    
  
It's possible, yes.  Is there a bug number?

    
    

    
  
Bug 268218.

    
    

    
  
This last patch also caused bug 268090, apparently.

    
    

    
  
Any chance that patch could also be responsible for Mac bug 268252? The timing
seems right, although the symptoms are somewhat different.

    
    

    
  
Yes, that's possible...
Depends on: 268090, 268218, 268252

    
    

    
  
Just in case if you will touch nsRegion.cpp in this or defect, please change
homegrown MIN_INT32 and MAX_INT32 to the ones from prtypes.h - PR_INT32_MIN and
PR_INT32_MAX respectively. They were added to NSPR after xp-regions were
implemented. Thanks!

    
    

    
  
This patch causes the Camino regression documented in bug 268218. Backing out
these changes fixes the regression.

    
    

    
  
Geoff, we're well aware of that and have been for a week.  Please do read bugs
before commenting on them.

    
    

    
  
(In reply to comment #65)
> Geoff, we're well aware of that and have been for a week.  Please do read bugs
> before commenting on them.

Boris,

Thanks for the gentle pointer. When I read the bug before reverting the patch
and testing with the two (patched and reverted) builds, the only comments I saw
in the bug addressing the regression in 268218 were #57, #58 and #59.
Specifically, when you said "It's possible, yes." I took that to mean that this
patch might be causing the regression but that it was unconfirmed. Please use
stronger language if you're well aware that this is the case. It may even be a
good idea to post that you've already confirmed this. Otherwise it seems that
testing and confirming (that this is the cause of the regression) adds new
information to the bug and should be posted. My apologies for the extra bugmail
if this confirmation was only new to me :-)
Blocks: 236548

    
    

    
  
Should this really be marked fixed with some depending bugs open?

    
    

    
  
Yes.
Blocks: 203439

    
  
No longer blocks: 21762
Blocks: 21762
Component: Layout: View Rendering → Layout: Web Painting

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: