424423 - Border rendering is slow

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The current border rendering algorithm works roughly like this:

- We create a clip region that includes all 4 corners and any sides that are not dotted or dashed.

- Depending on the style of each of the 4 sides, we decide whether to draw the border once (all sides same style), twice (top/left and bottom/right are the same, as in inset/outset), or four times (everything else).

- If we don't have any alpha in the colors, and we have to do more than one pass, we clear out the area of the destination we're drawing to, and set CAIRO_OPERATOR_ADD (to get the joins looking correct with partial coverage).

- If we do have alpha in the colors, then we use PushGroup() to create a temporary surface.  This is where most of the problems lie -- we create a temporary the size of the entire border area (intersected by the original dirty rect, so it's not going to be the full border area unless it's all visible on the screen).  But, in many cases, such as borders around the entire content, this is a huge area.

- We then do the regular drawing (1x, 2x, or 4x).  More problems arise here, due to the way drawing is done in cairo -- if the resulting drawing would cover anything other than integer aligned whole pixels (that is, if it can't be decomposed into rectangles), then we do the full rendering path, which involves creating a mask for the path -- but the mask will be the full extents of the path, even if we're only drawing just towards the outside.

- If we had dashed/dotted sides, we draw those sides.

- If we had to do a compositing group, we draw it back into the context.


There are a couple of big problems there, the main one being the sizes of the temporary surface that might be created, either at the PushGroup() step or the path step.  The 1x case is optimized further than what's described above, so not much should change there.  What we should be doing is much simpler:

- If we can draw the border in one go, just do it.

- Consider each of the four corners.  Set a clip for the corner only, on integer pixel coordinates.  If the adjacent styles are different: use PushGroup if necessary, set OPERATOR_ADD.  Draw the corner.

- Then set a clip region for each of the four sides (again, on integer aligned coordinates so that we don't have to deal with antialiasing issues, and so that the clipping is fast and doesn't require a clip mask), and draw each side.

Preemptively marking this blocking1.9- -- I'm going to work on this as I have time, and if I have something done for 1.9 we should switch, but I don't think we should block on this.

Comment 1

[Murilo Saraiva de Queiroz]

bump

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #1)
> bump

Don't do that.

Comment 3

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.

Comment 4

[Geva Zeichner]

I think this bug should be marked as a regression and a blocker. A lot of people will see this problem (of slow and jerky scrolling) as a very serious disadvantage of Firefox over other browsers. Especially people with slow computers, who are suffering from this the most.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3.0. A patch with tests may be considered for a 3.0.x release.

Comment 7

[Damon Sicore (:damons)]

Wouldn't block 1.9.1, but I think should be a high priority.

Vlad, you up for this?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 1) Add rounded rectangle API to thebes

This patch adds code to Thebes to render rounded rectangles, with the corners given in a new gfxCornerSizes structure.  The border radius code will depend on this (incidentally, with some parser fixes, we can support CSS3 border-radius with separate w/h).

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 328427 [details] [diff] [review]
(Part 1) Add rounded rectangle API to thebes

>+    void RoundedRectangle(const gfxRect& rect, const gfxCornerSizes& corners, PRBool cw = PR_TRUE);

Please either rename cw to something like corners_clockwise, add a comment to that effect, or both.
 
>+
>+struct THEBES_API gfxCorner {
>+    enum {
>+        // this order is important!
>+        TOP_LEFT = 0,
>+        TOP_RIGHT = 1,
>+        BOTTOM_RIGHT = 2,
>+        BOTTOM_LEFT = 3
>+    };
>+};
>+

Even though there are only 4 corners possible, for cleanliness I usually prefer to put NUMBER_OF_CORNERS as a sentinel value at the end of enums like this, and then use that value in loops.
 
>+    gfxPoint Corner(int corner) const {

Don't really like that int corner being passed in - a named enum would be nicer. It's not clear to me on how to combine that with using gfxCorner as a scope, though.

>+        switch (corner) {
>+            case gfxCorner::TOP_LEFT: return TopLeft();
>+            case gfxCorner::TOP_RIGHT: return TopRight();
>+            case gfxCorner::BOTTOM_LEFT: return BottomLeft();
>+            case gfxCorner::BOTTOM_RIGHT: return BottomRight();

default:  NS_WARNING("Invalid corner type");

Or maybe NS_ERROR?

>+
>+    gfxCornerSizes (gfxFloat v) {
>+        for (int i = 0; i < 4; i++)

Better to use a size sentinel enum value here.

>+    gfxCornerSizes (gfxFloat tl, gfxFloat tr, gfxFloat br, gfxFloat bl) {
>+        sizes[0].SizeTo(tl, tl);
>+        sizes[1].SizeTo(tr, tr);
>+        sizes[2].SizeTo(br, br);
>+        sizes[3].SizeTo(bl, bl);
>+    }
>+
>+    gfxCornerSizes (const gfxSize& tl, const gfxSize& tr, const gfxSize& br, const gfxSize& bl) {
>+        sizes[0] = tl;
>+        sizes[1] = tr;
>+        sizes[2] = br;
>+        sizes[3] = bl;
>+    }

Please use the symbolic names in the enumerated type above instead of 0...3.

>+    const gfxSize& operator[] (int index) const {
>+        return sizes[index];
>+    }
>+
>+    gfxSize& operator[] (int index) {
>+        return sizes[index];
>+    }

If there's any way to get type safety above, should change this.

>+    twoFloats cwCornerMults[4] = { { -1,  0 },
>+    twoFloats ccwCornerMults[4] = { { +1,  0 },
>+    for (int i = 0; i < 4; i++) {
>+        int c = cw ? ((i+1) % 4) : ((4-i) % 4);
>+        int i2 = (i+2) % 4;
>+        int i3 = (i+3) % 4;

There are several hardcoded 4s here that should probably be replaced with the symbol.

Other than that, looks good. r=JOEDREW! with those changes.

What's our testing like in this area?

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 1) Add rounded rectangle API to thebes (v2)

Updated based on review comments, carrying over r+.  Type safety with enums is a pain, because things like i++ don't work all that well on enum types.  I cheat a bit here and give it a name, but it's still an int :(  Made the other changes.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 2) Speed up rendering

Ok, the real deal now.  Comments in nsCSSRenderingBorders.{cpp,h} about how things are supposed to work.

Comment 12

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

+    twoFloats cwCornerMults[4] = { { -1,  0 },
+                                   {  0, -1 },
+                                   { +1,  0 },
+                                   {  0, +1 } };
+    twoFloats ccwCornerMults[4] = { { +1,  0 },
+                                    {  0, -1 },
+                                    { -1,  0 },
+                                    {  0, +1 } };

Those should be static and const.

Comment 13

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

CSSBorderRenderer should be "nsCSSBorderRenderer". Also I'd really like you to use the "a" naming convention for parameters; I hate it, but I lost the battle.

+      static nsBorderColors *noColors[4] = { NULL };

This should be const. In fact all the pointers to input stuff you're not modifying should be pointers to const, and so should the fields.

+  // core app units per pixel
+  nscoord mAUPP;

Make it PRInt32.

+  } SideClipType;

Why is this exposed in the header file since it's not used in the interface?

+  if (vals[0] == k &&
+      vals[1] == k &&
+      vals[2] == k &&
+      vals[3] == k)
+    return PR_TRUE;
+
+  return PR_FALSE;

why isn't this just "return ...;"?

Why don't we just do ComputeBorderCornerDimensions unconditionally somewhere instead of making it lazy?

+CSSBorderRenderer::AreBorderSideFinalStylesSame (PRUint8 whichSides)

No space before (

+  if (whichSides == 0 || (whichSides & ~SIDE_BITS_ALL) != 0) {
+    NS_WARNING("AreBorderSidesSame: invalid whichSides!");
+    return PR_FALSE;
+  }

Why not just an NS_ASSERTION and no return?

+    if (mBorderStyles[firstStyle] != mBorderStyles[i] ||
+        mBorderColors[firstStyle] != mBorderColors[i])
+      return PR_FALSE;

Should we really check when firstStyle is 0 even if side 0 is skipped?

+    // Check if all composite colors are the same, if they're present
+    if (mCompositeColors[firstStyle] && mCompositeColors[i]) {
+      nsBorderColors *col0 = mCompositeColors[firstStyle];
+      nsBorderColors *coli = mCompositeColors[i];

Use a helper function, it would help readability.

DoCornerClipSubPath and DoSideClipWithoutCornersSubPath can be simplified using lookup tables and helper functions. For DoCornerClipSubPath you just need a direction vector table indexed by corner ID with 0/1 entries that you multiply by (outsideRect.width - cornerDimensions.width) or (outsideRect.height - cornerDimensions.height).

+  if (side == NS_SIDE_TOP)
+    mContext->Rectangle(gfxRect(mOutsideRect.pos.x + mBorderCornerDimensions[C_TL].width,
+                                mOutsideRect.pos.y,
+                                mOutsideRect.size.width - (mBorderCornerDimensions[C_TL].width + mBorderCornerDimensions[C_TR].width),
+                                mBorderCornerDimensions[C_TL].height));

I'm bothered by the fact that this doesn't mention [C_TR].height. Shouldn't the height here actually be the border-top width?

+    switch (whichSide) {
+      case NS_SIDE_TOP:
+        start[0] = oRect.TopLeft();
+        start[1] = iRect.TopLeft();
+        break;

Can't we use gfxRect::Corner here?

Surely the 'start' and 'end' code here can be shared instead of duplicated.

+      mContext->MoveTo(oRect.BottomLeft() + gfxSize(0.5, 0.0));
+      mContext->LineTo(oRect.TopLeft() + gfxSize(0.5, 0.5));
+      mContext->LineTo(oRect.TopRight() + gfxSize(0.0, 0.5));

Would be slightly simpler to use a temporary rect with Inset(/*4param*/) and use its three corners.

+CSSBorderRenderer::MakeBorderColor(gfxRGBA& color, const gfxRGBA& backgroundColor, BorderColorStyle bpat)
+{

Make this just return the resulting color directly. Then you can use gfxRGBA constructors instead of setting each field.

+    while (lineIndex--)
+      borderColors = borderColors->mNext;

Yikes, so we're O(N^2) in the number of colors? Probably doesn't matter in practice but why aren't the colors in an array?

+      outColor.r = outColor.g = outColor.b = outColor.a = 0.0;

outColor should be the function result, not an out parameter. Too much XPCOM exposure, man. And gfxRGBA(0.0, 0.0, 0.0, 0.0) is more readable than this.

I'd actually take all the non-public static methods out of the class and make them static functions. You get better warnings if something becomes unused, slightly better optimization, and less .h file exposure.

+      if (borderColorStyle[0] != BorderColorStyleNone) {
+        ComputeColorForLine(1,
+                            borderColorStyle, borderColorStyleCount,
+                            nsnull, 0,
+                            borderRenderColor, mBackgroundColor, color);
+
+        soRect = mOutsideRect;
+        siRect = mInsideRect;
+
+        soRect.Inset(outerBorderWidths);
+
+        FillSolidBorder(soRect, siRect, radii, innerBorderWidths, sides, color);
+      }

Helper function for this lot please

+  BorderColorStyle *borderColorStyle = nsnull;

Use nsAutoArrayPtr

+  if (side == NS_SIDE_TOP) {

More table lookup love

+    length = start.y - end.y;

What's length for? It's unused

+       mBorderColors[0] == 0x00000000))

NS_RGBA(0,0,0,0)?

+    const PRIntn sidesForCorner[4][2] = {

static ... but do we really need it? can't we just do some math to find the two adjacent sides? Put it in a helper function if it makes the code hard to read

In DrawBorders where we draw sides and corners separately, do we get seams when borders aren't able to be pixel-aligned?

Comment 14

[Michael Ventnor]

It would also be good if you could update PaintBoxShadow to use the new rounded rectangle API if that would cause speed improvements.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 2) Speed up rendering (v2)

Some comments.. fixed all the other stuff:

> +    if (mBorderStyles[firstStyle] != mBorderStyles[i] ||
> +        mBorderColors[firstStyle] != mBorderColors[i])
> +      return PR_FALSE;
> 
> Should we really check when firstStyle is 0 even if side 0 is skipped?

We don't -- there's a little part at the start of the loop that keeps
incrementing firstStyle if the corresponding bit is 0.

> DoCornerClipSubPath and DoSideClipWithoutCornersSubPath can be simplified using
> lookup tables and helper functions. For DoCornerClipSubPath you just need a
> direction vector table indexed by corner ID with 0/1 entries that you multiply
> by (outsideRect.width - cornerDimensions.width) or (outsideRect.height -
> cornerDimensions.height).

DoCornerclipSubPath is easy; DoSideClipWithoutCornersSubPath is much harder.  I simplified
it some, but there are just too many things in play, I think, to make a lookup table
version make any sense.  The problem is that the size of the side has to be calculated here,
whereas the size of the rectangle is readily available in DoCornerClipSubPath.

> +  if (side == NS_SIDE_TOP)
> +    mContext->Rectangle(gfxRect(mOutsideRect.pos.x +
> mBorderCornerDimensions[C_TL].width,
> +                                mOutsideRect.pos.y,
> +                                mOutsideRect.size.width -
> (mBorderCornerDimensions[C_TL].width + mBorderCornerDimensions[C_TR].width),
> +                                mBorderCornerDimensions[C_TL].height));
> 
> I'm bothered by the fact that this doesn't mention [C_TR].height. Shouldn't the
> height here actually be the border-top width?

Mmm... yeah, that's probably clearer, and likely more correct.  The
border corner dimensions take into account the size of the sides, so
mBorderCornerdimensions[C_TL].height and [C_TR].height were going to be
at least the size of the top width.  But using the width directly is the
right way to go.

> +    while (lineIndex--)
> +      borderColors = borderColors->mNext;
> 
> Yikes, so we're O(N^2) in the number of colors? Probably doesn't matter in
> practice but why aren't the colors in an array?

nsBorderColors came deep from the bowels of the style system; didn't really want
to mess with it.  I guess I can put the colors in an array in the renderer code,
but that stuff is so infrequently used that it's not worth it I don't think.

> +      if (borderColorStyle[0] != BorderColorStyleNone) {
> +        ComputeColorForLine(1,
> +                            borderColorStyle, borderColorStyleCount,
> +                            nsnull, 0,
> +                            borderRenderColor, mBackgroundColor, color);
> +
> +        soRect = mOutsideRect;
> +        siRect = mInsideRect;
> +
> +        soRect.Inset(outerBorderWidths);
> +
> +        FillSolidBorder(soRect, siRect, radii, innerBorderWidths, sides,
> color);
> +      }
> 
> Helper function for this lot please

Not sure I can, not easily.  This is th eonly thing that I didn't really change, not sure if it was worth it.

> +    const PRIntn sidesForCorner[4][2] = {
> 
> static ... but do we really need it? can't we just do some math to find the two
> adjacent sides? Put it in a helper function if it makes the code hard to read

Why, though?  It's a simple little struct that does exactly what it says...

> In DrawBorders where we draw sides and corners separately, do we get seams when
> borders aren't able to be pixel-aligned?

Good catch.. I gotta start zooming in/out when I test stuff.  Luckily,
rounding up the corners to integer sizes fixes this.  It doesn't
matter if the corners are a bit bigger than exactly necessary, since
they're rendered in the same way as the actual sides, just with more
complex compositing.

Comment 16

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

+  PRBool allBordersSame = AreBorderSideFinalStylesSame (SIDE_BITS_ALL);

"space before (" is a disease!

+  if (side == NS_SIDE_TOP || side == NS_SIDE_BOTTOM) {
+    mContext->Rectangle(gfxRect(mOutsideRect.pos + offset,
+                                gfxSize(adjustedRectSize.width, mBorderWidths[side])));
+  } else {
+    mContext->Rectangle(gfxRect(mOutsideRect.pos + offset,
+                                gfxSize(mBorderWidths[side], adjustedRectSize.height)));
+  }

Just a little bit more:
  
  if (...) {
    adjustedRectSize.height = mBorderWidths[side];
  } else {
    adjustedRectSize.width = mBorderWidths[side];
  }
and maybe rename adjustedRectSize to just 'size'.

I think that nsCSSBorderRenderer::DoSideClipSubPath can be simplified more. startIsDashed/startType/start and endIsDashed/endType/end are computed pretty much the same way, just with different parameters. A helper function should share them.

> Not sure I can, not easily.  This is th eonly thing that I didn't really
> change, not sure if it was worth it.

Why's it hard? Something non-static like

void FillSolidBorderLine(const BorderColorStyle* aBorderColorStyle,
                         PRInt32 aBorderColorStyleCount,
                         PRInt32 aLineIndex,
                         nscolor aBorderRenderColor,
                         gfxFloat* aBorderWidths, /* 4 x aBorderColorStyleCount */
                         const gfxCornerSizes& aRadii,
                         PRIntn aSides);

I think that should work.

+    if (borderColorStyle[0] != BorderColorStyleNone) {
+      color = ComputeColorForLine(2,

Looking at this again, I'm not sure why we test style 0 and then use style 2.

Some of your functions (ComputeBorderCornerDimensions, ComputeInnerRadii, AreCompositeColorsEqual, DoSideClipWithoutCornersSubPath, MaybeMoveToMidPoint, DrawBorderSidesCompositeColors) still need "a" treatment. Sorry :-(

+  if (aSide == NS_SIDE_TOP) {
+    start = gfxPoint(mOutsideRect.pos.x + mBorderCornerDimensions[C_TL].width,
+                     (mOutsideRect.pos.y + mInsideRect.pos.y) / 2.0);
+    end = gfxPoint(mOutsideRect.pos.x + mOutsideRect.size.width - mBorderCornerDimensions[C_TR].width,
+                   (mOutsideRect.pos.y + mInsideRect.pos.y) / 2.0);
+  } else if (aSide == NS_SIDE_RIGHT) {

This stuff doesn't look any simpler than in the last patch.

> Why, though?  It's a simple little struct that does exactly what it says...

We've got similar math everywhere else, why not here? Here, I'll write it for you :-):

    const PRIntn sides[2] = { corner, (corner+3)%4 };

although we probably should use &3 instead of %4 everywhere.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> > Not sure I can, not easily.  This is th eonly thing that I didn't really
> > change, not sure if it was worth it.
> 
> Why's it hard? Something non-static like
> 
> void FillSolidBorderLine(const BorderColorStyle* aBorderColorStyle,
>                          PRInt32 aBorderColorStyleCount,
>                          PRInt32 aLineIndex,
>                          nscolor aBorderRenderColor,
>                          gfxFloat* aBorderWidths, /* 4 x aBorderColorStyleCount
> */
>                          const gfxCornerSizes& aRadii,
>                          PRIntn aSides);
> 
> I think that should work.

The problem is that the parameters aren't the same -- e.g. if you look at the 3-style case, the outer/inner rects are inset by:

1) inner by innerBorderWidths + middleBorderWidths
2) outer by outerBorderWidths, inner by innerBorderWidths,
3) outer by outer + middle

The radii are progressively shrunk as well.

Or are you just suggesting merging the FillSolidBorder and ComputeColorForLine?  That doesn't seem that much better.  I'm looking at how to maybe pass down everything needed to another function, but even that just seems like creating a function with a huge number of args.  Originally I wanted to write this as a loop from 0 to the number of render styles... maybe I can do taht with some special-casing inside the loop.


> +    if (borderColorStyle[0] != BorderColorStyleNone) {
> +      color = ComputeColorForLine(2,
> 
> Looking at this again, I'm not sure why we test style 0 and then use style 2.

Hrm.  That's a bug, I wonder what happened there.  It doesn't change any of the rendering since the only place it's skipped is in the 3 case where the middle style is None; I'll fix the conditions. 

> +  if (aSide == NS_SIDE_TOP) {
> +    start = gfxPoint(mOutsideRect.pos.x + mBorderCornerDimensions[C_TL].width,
> +                     (mOutsideRect.pos.y + mInsideRect.pos.y) / 2.0);
> +    end = gfxPoint(mOutsideRect.pos.x + mOutsideRect.size.width -
> mBorderCornerDimensions[C_TR].width,
> +                   (mOutsideRect.pos.y + mInsideRect.pos.y) / 2.0);
> +  } else if (aSide == NS_SIDE_RIGHT) {
> 
> This stuff doesn't look any simpler than in the last patch.

Well, it's shorter :) I gave up on trying to figure out how to split it up more, I don't think it's worth it.

Fixed the other stuff, another patch coming up soon.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 2) Speed up rendering (v3)

Ok, fingers crossed!  Even more simplification. (Thanks for pushing me to simplify all this stuff!)

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Try server builds appearing here: https://build.mozilla.org/tryserver-builds/2008-07-14_16:[e-mail]-424423/

Can folks who were seeing slow rendering due to borders give these builds a try?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I can't run those builds on the machine on which I was seeing the slowness without upgrading Linux on it first... well, and finding the box it's in and unpacking it.  :(  It's a lower priority than unpacking other things, unfortunately.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Unless those builds will run against gtk 2.8?  For a while there was talk of that being possible, I think...

Comment 22

[Brian Polidoro]

I tried the border benchmark test 
http://people.mozilla.com/~vladimir/misc/borderbench.html
and this made a world of difference on the two very slow cases:

Nightly
[251,266,230,**3099,5768**,270,295,312,522,741,490,488,267,574,411,588,261,317,229,252]
[240,244,251,**3114,5759**,275,294,312,533,760,503,492,228,588,411,593,271,309,226,232]

Tryserver
[204,244,318,**186,245**,229,222,249,264,547,228,241,258,267,258,242,284,358,262,254]
[226,223,249,**248,255**,263,250,373,367,544,258,260,243,253,258,248,288,354,268,255]
I'm on Windows XP.

Comment 23

[Brian Polidoro]

For another frame of reference though here's my FF2 numbers.

[140,159,171,139,141,138,173,171,187,170,141,172,156,141,139,172,0,0,0,0]
[158,141,156,143,157,126,172,126,188,156,155,140,172,155,171,124,0,0,0,0]

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(note that the FF2 and FF3 numbers aren't directly comparable, as the measurement methods are different -- ff2 uses scrolling whereas ff3 calls DOMWindowUtils' repaint() directly)

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 2) Speed up rendering (v4)

Just minor changes from v3; all reftests seem to happily pass, though this doesn't fix some of the opreator-ADD induced problems on OSX as mentioned by bug 379317.  I have a separate plan to fix those (by tweaking the reftest driver).

Comment 26

[u212413]

I installed the tryserver build, but scrolling is still _very_ jerky. :(

see bug 426907 which is marked (unjustified?) as a dupe of this bug:
https://bugzilla.mozilla.org/attachment.cgi?id=313479
https://bugzilla.mozilla.org/attachment.cgi?id=313474

my system is athlon xp 2600, win xp sp3. (fx2/opera/safari/IE scroll smooth)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comparing those tryserver builds to trunk Firefox, the tryserver builds seem a bit faster on bug 397303, maybe a bit faster on wikipedia (bug 405740), and about the same on tinderbox.

In all three cases, neither build "keeps up" with the scrolling: if I hold down the down arrow for a bit, then let go, the scrolling doesn't stop until a lot later....

Comment 28

[Michael Ventnor]

Will this land soon? I am interested in starting bug 431176 but would like this patch in moz-central first, and I don't have a lot of time left in my holidays to code stuff.

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: (Part 3) Layer more optimizations on top

Some more layered optimizations.  With this patch, every benchmark in borderbench is either the same (pure solid borders, which were already fast) or significantly faster.  This fixes a few things that I noticed:

- Clipping is expensive, even to pixel-aligned rectangles... more so than I'd expect it to be.  This really should be looked into in a separate bug.

- If we don't have border-radius, it's worth doing some side-munging to just draw a few rectangles, one for each side.  It also turns out that for (side) { NewPath(); Rectangle(r[side]); Fill(); } is faster than filling a single path with all four rectangles in it.

Without this, there are a few regressions in perf for a few test cases, caused by my overaggressive code simplification earlier, so I don't want to check in the earlier work until this can land at the same time.

Tryserver builds with this (and a new cairo) are available at https://build.mozilla.org/tryserver-builds/2008-07-19_18:[e-mail]-jumbo2/ .

Comment 30

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

+  PRPackedBool mOneUnitBorder : 1;
+  PRPackedBool mNoBorderRadius : 1;

Bitfields are really unnecessary here, let's not use them.

+  // then fix up corners

You should say that the goal here is to avoid overlapping rectangles (this matters for rgba() of course).

(You knew I was going to say this) I wish the r[] stuff in nsCSSBorderRenderer::DoCornerSubPath was simpler. What if we just took the RoundedRectangle path for aSides != SIDE_BITS_ALL? aSides != SIDE_BITS_ALL should be very rare, I think; if I'm right, we shouldn't bother optimizing it for all and we can initialize r much more easily. In fact you can probably just write down 4 FillRect calls.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

I was looking through the mozilla-central changes and noticed the RoundedRectangle code. Is there are reason this code doesn't just use cairo_arc() instead of doing it's own thing to draw arcs with bezier curves?

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #31)
> I was looking through the mozilla-central changes and noticed the
> RoundedRectangle code. Is there are reason this code doesn't just use
> cairo_arc() instead of doing it's own thing to draw arcs with bezier curves?

Yep, cairo_arc can't do elliptical arcs -- could do it by munging the matrix, but then it would need to be saved/restored.  Internally it all ends up as a curve anyway, so it was more straightforward to just calculate it directly.

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This landed, as joe just reminded me.

Comment 34

[Tony Chung [:tchung]]

how can this be tested?   are there tests checked in for this patch?

Comment 35

[u212413]

check this out:

http://people.mozilla.com/~vladimir/misc/borderbench.html
https://bugzilla.mozilla.org/show_bug.cgi?id=426907 (testcases)

Comment 36

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090217 Minefield/3.2a1pre. 

Are we planning to get this fix into 1.9.1?

Comment 37

[u212413]

Please note that this problem still exists on relatively slow (win?) machines. The patch doesn't help much in testcases which affect scrolling performance, like bug 426907.

Comment 38

[Murilo Saraiva de Queiroz]

Scrolling (specially in GMail) using Firefox 3.1b2 (and many other previous versions) is still extremely slow in my machine (Pentium 4 HT 2.4 GHz with 1.5 GB RAM, onboard Intel 82965G video). 

Scrolling it's 2-3 slower (sometimes worse) than Firefox 2 in exactly the same configuraton, using brand new, clean profiles. 

I'm using Ubuntu 8.10 "Intrepid Ibex"

Comment 39

[u212413]

Murilo: The same problem as I described in bug 426907 (marked as a dupe of this bug). Could anyone reopen it?

Comment 40

[Tony Chung [:tchung]]

(In reply to comment #39)
> Murilo: The same problem as I described in bug 426907 (marked as a dupe of this
> bug). Could anyone reopen it?

I've tried dragging the two testcases up and down in bug 426907, and the performance looks identical to me.  

Running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090302 Minefield/3.2a1pre 
and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090226 Shiretoko/3.1b3pre

Can you verify this is still the same on a nightly for you?   If so, perhaps a video screencast showing your behavior, as well as environment specs?