516740 - Painting takes too long if also updating innerHTML

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I just tried taking out the innerHTML updates on the testcase for bug 424715 and discovered (with my various layout patches to make the O(N^2) behavior go away) that painting takes a lot less time that way (15s runtime instead of 65s).  I thought this might have been due to us simplifying paint regions, but I tried changing MAX_RECTS_IN_REGION in nsChildView.mm to 1000 and changed the SimplifyOutward calls in nsViewManager::UpdateWidgetArea and AccumulateIntersectionsIntoDirtyRegion to use 1000 as well, and that seems to have no effect.

It'd be nice to figure out why we're painting as much as we are here.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The damage region we receive for the paints is the entire window, which is pretty disturbing.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And that's what we get from the system...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I lied, here is the sort of thing we get at each frame:

Breakpoint 3, nsChildView::Invalidate (this=0xc91b1c0, aRect=@0xbfffbde4, aIsSynchronous=0) at /Users/roc/mozilla-checkin/widget/src/cocoa/nsChildView.mm:1811
1811	  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
$527 = (const nsIntRect & volatile) @0xbfffbde4: {
  x = 769, 
  y = 0, 
  width = 383, 
  height = 15
}
Current language:  auto; currently objective-c++
Warning: the current language does not match this frame.

Breakpoint 3, nsChildView::Invalidate (this=0xc91b1c0, aRect=@0xbfffbd64, aIsSynchronous=0) at /Users/roc/mozilla-checkin/widget/src/cocoa/nsChildView.mm:1811
1811	  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
$528 = (const nsIntRect & volatile) @0xbfffbd64: {
  x = 769, 
  y = 0, 
  width = 383, 
  height = 15
}

Breakpoint 3, nsChildView::Invalidate (this=0xc91b1c0, aRect=@0xbfffbf34, aIsSynchronous=0) at /Users/roc/mozilla-checkin/widget/src/cocoa/nsChildView.mm:1811
1811	  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
$529 = (const nsIntRect & volatile) @0xbfffbf34: {
  x = 769, 
  y = 0, 
  width = 383, 
  height = 15
}

Breakpoint 3, nsChildView::Invalidate (this=0xc91b1c0, aRect=@0xbfffbf34, aIsSynchronous=0) at /Users/roc/mozilla-checkin/widget/src/cocoa/nsChildView.mm:1811
1811	  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
$530 = (const nsIntRect & volatile) @0xbfffbf34: {
  x = 769, 
  y = 14, 
  width = 383, 
  height = 1
}

Breakpoint 3, nsChildView::Invalidate (this=0xc91b1c0, aRect=@0xbfffbf34, aIsSynchronous=0) at /Users/roc/mozilla-checkin/widget/src/cocoa/nsChildView.mm:1811
1811	  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
$531 = (const nsIntRect & volatile) @0xbfffbf34: {
  x = 454, 
  y = 233, 
  width = 244, 
  height = 10
}

Breakpoint 2, nsViewManager::Refresh (this=0xfcf5430, aView=0xd843bf0, aContext=0xe5729e0, aRegion=0xe5734c0, aUpdateFlags=1) at /Users/roc/mozilla-checkin/view/src/nsViewManager.cpp:436
436	  if (damageRegion.IsEmpty()) {
$532 = {
  mRectCount = 1, 
  mCurRect = 0x10c2c5c, 
  mRectListHead = {
    <nsRegion::nsRectFast> = {
      <nsRect> = {
        x = 0, 
        y = 0, 
        width = 0, 
        height = 0
      }, <No data fields>}, 
    members of nsRegion::RgnRect: 
    prev = 0x10c2c5c, 
    next = 0x10c2c5c
  }, 
  mBoundRect = {
    <nsRect> = {
      x = 46140, 
      y = 0, 
      width = 22980, 
      height = 900
    }, <No data fields>}
}
Current language:  auto; currently c++
$533 = {
  <nsRegion::nsRectFast> = {
    <nsRect> = {
      x = 46140, 
      y = 0, 
      width = 22980, 
      height = 900
    }, <No data fields>}, 
  members of nsRegion::RgnRect: 
  prev = 0xbfffc0a8, 
  next = 0xbfffc0a8
}
$534 = {
  <nsRegion::nsRectFast> = {
    <nsRect> = {
      x = 0, 
      y = 0, 
      width = 0, 
      height = 0
    }, <No data fields>}, 
  members of nsRegion::RgnRect: 
  prev = 0x10c2c5c, 
  next = 0x10c2c5c
}

Breakpoint 2, nsViewManager::Refresh (this=0xfcf5430, aView=0xd843bf0, aContext=0xe0a48f0, aRegion=0xe572960, aUpdateFlags=1) at /Users/roc/mozilla-checkin/view/src/nsViewManager.cpp:436
436	  if (damageRegion.IsEmpty()) {
$535 = {
  mRectCount = 2, 
  mCurRect = 0x10c2aac, 
  mRectListHead = {
    <nsRegion::nsRectFast> = {
      <nsRect> = {
        x = 0, 
        y = 2147483647, 
        width = 0, 
        height = 0
      }, <No data fields>}, 
    members of nsRegion::RgnRect: 
    prev = 0x10c2aac, 
    next = 0x10c2c14
  }, 
  mBoundRect = {
    <nsRect> = {
      x = 27240, 
      y = 0, 
      width = 41880, 
      height = 14580
    }, <No data fields>}
}
$536 = {
  <nsRegion::nsRectFast> = {
    <nsRect> = {
      x = 46140, 
      y = 0, 
      width = 22980, 
      height = 900
    }, <No data fields>}, 
  members of nsRegion::RgnRect: 
  prev = 0xbfffaf38, 
  next = 0x10c2aac
}
$537 = {
  <nsRegion::nsRectFast> = {
    <nsRect> = {
      x = 27240, 
      y = 13980, 
      width = 14640, 
      height = 600
    }, <No data fields>}, 
  members of nsRegion::RgnRect: 
  prev = 0x10c2c14, 
  next = 0xbfffaf38
}

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We get 4 invalidates associated with the text, followed by 1 invalidate of the pixel strip, which is 10 pixels high. Then we get two refreshes, the first one is just for the text, then again for the text plus pixel strip.

Here's why we see two refreshes here ... there's a pending paint event for the invalidated text and a pending reflow for the abs-pos elements in the pixel strip (and possibly also a pending reflow for the text, but that doesn't matter). The repaint event fires and we get into nsViewManager::DispatchEvent which flushes reflows, which triggers another invalidate which leads to the second refresh.

I'm not sure why the bounding box for the reflowed stuff is 10 pixels high, need to look into that.

Dumping display lists, it's also clear that we're painting every single abs-pos pixel-rect in the bounding-box of the dirty region, even though we optimize the display list which should be throwing out elements that aren't in the region, looking into that now.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we're failing to optimize here because we're subtracting the opaque pixel rects from the region until the region gets too complex, and then it fluffs out to its bounding-box. If I remove the code that subtracts the bounding box from the region, we only paint a reasonable subset of the abs-pos elements. (However, my build does have the patches from bug 513082 in it, so something slightly different may be going on on trunk).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  I thought I had hacked most of the fluff-out things to not do it (per comment 0), but maybe I missed one?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the 10-pixels-high dirty rect is just 3 rows of pixels plus 1 pixel of slop due to the abs-pos rects not being pixel-aligned, so nothing particularly wrong there.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> Hmm.  I thought I had hacked most of the fluff-out things to not do it (per
> comment 0), but maybe I missed one?

Maybe, I dunno.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch

This fixes it for me. Seems like the right thing to do, because it ensures that we never increase the area of the visible region.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 400909 [details] [diff] [review]
patch

This is obviously good, but doesn't make the testcase better on its own for me...

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe it's the combination with the patches for bug 513082. Hopefully that will get reviewed and we can get all this on trunk and see how we stand.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sure, let's try that.

Comment 13

[Markus Stange [:mstange]]

There's another reason for redrawing the entire window: focus rings. Bug and patch coming soon.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/385f917b3cf9

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 400909 [details] [diff] [review]
patch

This is a simple safe fix that we should probably take on 1.9.2.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 400909 [details] [diff] [review]
patch

a1.9.2=dbaron

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18ca526ddff1