The profiles show a great deal of time being spent rescaling the image.

Joe and I worked down a regression range on the 1.9.0 branch. The 2008-08-25 Mac nightly is fast on the testcase, the 2008-08-26 Mac nightly is slow. I padded the range a bit to account for build time, and it gives us this range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-08-25+01%3A00%3A00&maxdate=2008-08-26+04%3A00%3A00&cvsroot=%2Fcvsroot That range only includes one unlikely libpr0n change, but does include several layout changes. Among them: Bug 449362 - [FIX]Table border is rendered incorrectly when the table cell size changes due to javascript. This may be a blind alley or only half the problem, but when I saw that, I tried modifying the testcase html to remove the table that wraps the status text field. With that change, it becomes quite responsive.

(On the subject of tables, bug 423823 is also in the range)

Note that the slowness issue here is that the image is being painted over and over, even though that portion of the page isn't changing at all. I'm not sure why the image is being so slow to paint (even scaled), but regardless, we should not be repainting it at all.

Right. There are two bugs here: that the page is painting over and over, and that the image is resampling every time it redraws.

Removing the PushGroup in nsThebesImage (just #if 0'd out the whole block) that we use to work around bug 364968 makes CGImage apparently not discard its cached version, because typing is fast in a build with that workaround removed. Jeff discovered/intuitited that Quartz's resized cache image is stored on the surface, and PushGroup causes Cairo to create a whole new surface that then gets discarded by PopGroup. Of course, this whole codepath wouldn't be used except that the translations on the matrix have an x component of something like -80,000. Still unknown is why that is the case.

OK. So on every character typed we repaint the textarea, the "What are you doing?" text, the number of characters left, and the entire right sidebar. We do the same thing on any focus in or out. Looking into why.

If you delete the "More" element, then we seem to stop doing the full repainting. Also if you delete the twitter chunk right above the More element.

So during each reflow (and I haven't looked into why there are reflows on focus, but I assume the site has JS that does something), we have a table cell that changes height twice. The first time from 259.1166666666px to 251.8 px, with the CheckInvalidateSizeChange stack looking like: #0 nsIFrame::CheckInvalidateSizeChange (this=0x2011eae8, aOldRect=@0xbfff91d0, aOldOverflowRect=@0xbfff91c0, aNewDesiredSize=@0xbfff9044) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/generic/nsFrame.cpp:4039 #1 0x11e1ad1c in nsTableFrame::InvalidateFrame (aFrame=0x2011eae8, aOrigRect=@0xbfff91d0, aOrigOverflowRect=@0xbfff91c0, aIsFirstReflow=0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableFrame.cpp:6919 #2 0x11e34b90 in nsTableRowFrame::ReflowChildren (this=0x20120e44, aPresContext=0x1501600, aDesiredSize=@0xbfff9454, aReflowState=@0xbfff93ac, aTableFrame=@0x20120a1c, aStatus=@0xbfffa114) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowFrame.cpp:975 #3 0x11e33111 in nsTableRowFrame::Reflow (this=0x20120e44, aPresContext=0x1501600, aDesiredSize=@0xbfff9454, aReflowState=@0xbfff93ac, aStatus=@0xbfffa114) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowFrame.cpp:1069 #4 0x11c86e3d in nsContainerFrame::ReflowChild (this=0x20120c68, aKidFrame=0x20120e44, aPresContext=0x1501600, aDesiredSize=@0xbfff9454, aReflowState=@0xbfff93ac, aX=0, aY=0, aFlags=16, aStatus=@0xbfffa114, aTracker=0x0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/generic/nsContainerFrame.cpp:824 #5 0x11e3a853 in nsTableRowGroupFrame::ReflowChildren (this=0x20120c68, aPresContext=0x1501600, aDesiredSize=@0xbfff9814, aReflowState=@0xbfff95a0, aStatus=@0xbfffa114, aPageBreakBeforeEnd=0xbfff959c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowGroupFrame.cpp:427 The second time back to 259.1166666px, with this stack for the CheckInvalidateSizeChange call: #0 nsIFrame::CheckInvalidateSizeChange (this=0x2011eae8, aOldRect=@0xbfff9100, aOldOverflowRect=@0xbfff90f0, aNewDesiredSize=@0xbfff9074) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/generic/nsFrame.cpp:4039 #1 0x11e1ad1c in nsTableFrame::InvalidateFrame (aFrame=0x2011eae8, aOrigRect=@0xbfff9100, aOrigOverflowRect=@0xbfff90f0, aIsFirstReflow=0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableFrame.cpp:6919 #2 0x11e320de in nsTableRowFrame::DidResize (this=0x20120e44) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowFrame.cpp:386 #3 0x11e35ffa in nsTableRowGroupFrame::DidResizeRows (this=0x20120c68, aDesiredSize=@0xbfff9814) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowGroupFrame.cpp:557 #4 0x11e3a405 in nsTableRowGroupFrame::CalculateRowHeights (this=0x20120c68, aPresContext=0x1501600, aDesiredSize=@0xbfff9814, aReflowState=@0xbfff976c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowGroupFrame.cpp:844 #5 0x11e3abe9 in nsTableRowGroupFrame::ReflowChildren (this=0x20120c68, aPresContext=0x1501600, aDesiredSize=@0xbfff9814, aReflowState=@0xbfff95a0, aStatus=@0xbfffa114, aPageBreakBeforeEnd=0xbfff959c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/tables/nsTableRowGroupFrame.cpp:498 Now what I want to know is this... What does a trunk build from 2008-02-06 behave like on the testcase? Is it slow (with the 2008-02-07 build fast)?

Comment 7 exactly jibes with comment 8. The natural height of the left-hand cell there is a bit taller than the right-hand one (at least here, and presumably on vlad's machine). So when we reflow, we first set the right-hand cell to its natural height, then make it a bit taller so all the cells in the row are the same height. Deleting anything in the left-hand cell makes the right-hand cell the taller one, so its size doesn't change during the row-height calculation pass... The reason we have a reflow at all is because of some removeChild calls in the page JS: 0 anonymous() ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":12] this = [object HTMLSpanElement @ 0x1e57ba60 (native @ 0x1ec4fd00)] 1 anonymous(E = [object Object], [function], [object Object]) ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":12] I = undefined H = 1 G = 1 D = undefined this = function (D, E) { return new (n.fn.init)(D, E); } 2 anonymous(D = [object Object], E = [function]) ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":12] this = [object Object] 3 anonymous() ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":12] this = [object Object] 4 anonymous() ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_004.js":9] F = undefined this = [object Object] 5 anonymous(D = "140") ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":12] this = [object Object] 6 B() ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/application.js":1] K = 0 L = "" this = [object Window @ 0x1d0d5270 (native @ 0x1d0d44c0)] 7 anonymous(K = [object Object]) ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/application.js":1] this = [object HTMLTextAreaElement @ 0x1e577d30 (native @ 0x1d3a45b0)] 8 anonymous([object Object]) ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":19] E = undefined G = [function] F = "4" H = /(^|\.)(\.|$)/ K = this = [object HTMLTextAreaElement @ 0x1e577d30 (native @ 0x1d3a45b0)] 9 anonymous([object Event @ 0x1e57e360 (native @ 0x1d07f310)]) ["file:///Users/bzbarsky/test/twitter-slow-mkapor_files/jquery_003.js":19] this = [object HTMLTextAreaElement @ 0x1e577d30 (native @ 0x1d3a45b0)] Now the remaining question is why we end up invalidating the whole cell; let me dig into that.

Ah, in master.css we have this style for #side_base: border-left:1px solid #bddcad; and then in CheckInvalidateSizeChange we have this code: // Invalidate the old frame borders if the frame has borders. Those borders // may be moving. const nsStyleBorder* border = GetStyleBorder(); NS_FOR_CSS_SIDES(side) { if (border->GetActualBorderWidth(side) != 0) { Invalidate(nsRect(0, 0, aOldRect.width, aOldRect.height)); return; } } which is the case we hit here, of course. roc, do you think we could make this smarter and not either not invalidate as much, or not invalidate on vertical resize + vertical border? Of course ideally we'd not invalidate in the table code until we know whether we're "really" resizing, but that would require maintaining the intermediate sizing information somewhere other than the cells' mRect...

Yes, we can definitely make CheckInvalidateSizeChange smarter. Worth doing?

Need doing for a beta? If so, please mark this as a P1 blocker!

A fix for this is unlikely to need beta exposure.

I think making CheckInvalidateSizeChange smarter is probably worth it, yes.

We should do both fixes here, I think; both in nsThebesImage::Draw on the Mac if we can get rid of the pixman workaround, and the layout issue.

Test URL: jar:https://bug488901.bugzilla.mozilla.org/attachment.cgi?id=373370!/twitter-reduce.html

Fairly straightforward. It turns out we need to be careful to examine only the relevant corners to check for border-radius, because in the Twitter case it is actually using rounded corners on the right edge...

This allows us to take advantage of the scaled image cache attached to the CGContext.

Comment on attachment 375247 [details] [diff] [review] Don't use push group on OS X I think it would be cleaner and safer to make this a surface type test instead of an #ifdef.

Fixed to use a runtime check instead of a compile time one (a much better idea, thanks roc). I've also used a diff without whitespace differences to make it easier to review.

Comment on attachment 374705 [details] [diff] [review] fix >+// aCorner is a "full corner" value, i.e. NS_CORNER_TOP_LEFT etc >+static PRBool IsCornerAdjacentToSide(PRUint8 aCorner, PRUint8 aSide) >+{ >+ return aSide == aCorner || aSide == (aCorner - 1)&3; >+} Would you mind littering this with appropriate PR_STATIC_ASSERTs? >+/* static */ PRBool >+nsLayoutUtils::HasNonZeroCornerOnSide(const nsStyleCorners& aCorners, >+ PRUint8 aSide) >+{ >+ NS_FOR_CSS_HALF_CORNERS(corner) { >+ // corner is a "half corner" value, so dividing by two gives us a >+ // "full corner" value. >+ if (NonZeroStyleCoord(aCorners.Get(corner)) && It occurs to me that we don't really get rounding unless both radii for the ellipse are nonzero. (Is that correct?) I suspect we don't care, though. r+sr=dbaron

Pushed the pushgroup patch: http://hg.mozilla.org/mozilla-central/rev/d40853c4aacc

(In reply to comment #21) > (From update of attachment 374705 [details] [diff] [review]) > >+// aCorner is a "full corner" value, i.e. NS_CORNER_TOP_LEFT etc > >+static PRBool IsCornerAdjacentToSide(PRUint8 aCorner, PRUint8 aSide) > >+{ > >+ return aSide == aCorner || aSide == (aCorner - 1)&3; > >+} > > Would you mind littering this with appropriate PR_STATIC_ASSERTs? Willdo. > >+/* static */ PRBool > >+nsLayoutUtils::HasNonZeroCornerOnSide(const nsStyleCorners& aCorners, > >+ PRUint8 aSide) > >+{ > >+ NS_FOR_CSS_HALF_CORNERS(corner) { > >+ // corner is a "half corner" value, so dividing by two gives us a > >+ // "full corner" value. > >+ if (NonZeroStyleCoord(aCorners.Get(corner)) && > > It occurs to me that we don't really get rounding unless both radii for the > ellipse are nonzero. (Is that correct?) I suspect we don't care, though. Yes, I intend to not care about that.

Pushed my patch: http://hg.mozilla.org/mozilla-central/rev/3802713fbe2e

Landed Jeff's pushgroup patch on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6cfd3fbcab74

The code in nsThebesImage::Draw() is completely different from 1.9.1 but here's an untested patch that should accomplish basically the same thing.

Still slow for me in today's 1.9.1 nightly. I'll grab a shark in a bit, have to run an errand.

Nightly and trunk feel about the same for me on Mac, and both seem fast enough --- OK, it's a fast laptop, but it's a debug build. At least they're fast enough that holding a key down repeats without lagging at all.

Comment on attachment 378354 [details] [diff] [review] Something similar for 1.9.0 1.9.0.11 is code frozen for anything but critical, respin-inducing patches. Moving nomination out to 1.9.0.12. It'd be good to get this tested before we approve though.

Comment on attachment 378354 [details] [diff] [review] Something similar for 1.9.0 Approved for 1.9.0.12, a=dveditz for release-drivers

Checking in nsThebesImage.cpp; /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v <-- nsThebesImage.cpp new revision: 1.87; previous revision: 1.86 done

Had to back this out of 1.9.0 because it broke the tree.

Checking in nsThebesImage.cpp; /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v <-- nsThebesImage.cpp new revision: 1.89; previous revision: 1.88 done