Bug 39123 – Unneeded fillRect call in RenderBoxModelObject::paintFillLayerExtended

• Status: NEW
• Component: Layout and Rendering

[Pat Galizia]

For SVN version 59242 of RenderBoxModelObject (and earlier):

In the RenderBoxModelObject::paintFillLayerExtended method, there's a call to fillRect for the baseColor (line 587), then another one a few lines later for bgColor (line 594).  If the alpha of the bgColor is set to the max value (i.e., no blending), then the first fillRect for baseColor should be unnecessary.

Example: starting at line 581:
#####################################
        // If we have an alpha and we are painting the root element, go ahead and blend with the base background color.
        if (isOpaqueRoot) {
            Color baseColor = view()->frameView()->baseBackgroundColor();
            if (baseColor.alpha() > 0) {
                context->save();
                context->setCompositeOperation(CompositeCopy);
                context->fillRect(rect, baseColor, style()->colorSpace()); // Can we check here for bgColor's alpha, and drop the draw if 0xff?
                context->restore();
            } else
                context->clearRect(rect);
        }

        if (bgColor.isValid() && bgColor.alpha() > 0)
            context->fillRect(rect, bgColor, style()->colorSpace());  /// If this has alpha 0xff, for example, that should make the above
#####################################                  


We do have a proposed patch for that, which will be attached shortly.

Found on an ARM-based system, but should be applicable across all platforms.

[Pat Galizia]

Created attachment 56084 [details]
Proposed patch for this enhancement.  Eliminates one fillRect if possible.

[Darin Adler]

Comment on attachment 56084 [details]
Proposed patch for this enhancement.  Eliminates one fillRect if possible.

> +                if (!(bgColor.isValid() && (bgColor.alpha() == 0xff))) {

There's no need to check isValid here. Invalid colors have an alpha of 0.

> +                    context->save();
> +                    context->setCompositeOperation(CompositeCopy);
> +                    context->fillRect(rect, baseColor, style()->colorSpace());
> +                    context->restore();

Since this is a performance optimization, the data we need is how it affects performance. Have you measured faster speed in page rendering?

[Pat Galizia]

(In reply to comment #2)
> (From update of attachment 56084 [details])
> > +                if (!(bgColor.isValid() && (bgColor.alpha() == 0xff))) {
> 
> There's no need to check isValid here. Invalid colors have an alpha of 0.
> 

Okay -- I'll remove that.

> > +                    context->save();
> > +                    context->setCompositeOperation(CompositeCopy);
> > +                    context->fillRect(rect, baseColor, style()->colorSpace());
> > +                    context->restore();
> 
> Since this is a performance optimization, the data we need is how it affects performance. Have you measured faster speed in page rendering?

This was part of a larger series of changes done for Chromium.  I'll capture measurements for this change by itself.

[Pat Galizia]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 56084 [details] [details])
> > Since this is a performance optimization, the data we need is how it affects performance. Have you measured faster speed in page rendering?
> 
> This was part of a larger series of changes done for Chromium.  I'll capture measurements for this change by itself.

Ran the Chromium browser (using local snapshots of various pages with the Chromium Benchmark extension) showed minimal change -- well within the margin of error.

We then instrumented that block of code to measure the average execution time with and without the patch.  There was an improvement seen, but if depended on the particular webpage loaded (of course).

For example, cnn.com and apple.com page loads saw more improvement (1.7ms and 1.2ms respectively for that block), while sites like netflix.com and yahoo.com saw less (0.7ms and 0.1ms on average) on our 1GHz ARM device.

[Eric Seidel (no email)]

Comment on attachment 56084 [details]
Proposed patch for this enhancement.  Eliminates one fillRect if possible.

r- based on darin's comments above.

[Simon Fraser (smfr)]

I think it's OK to make a change like this even without hard performance data. We certainly know that FillRect is slow.

I would like to see a comment in the code to explain why the fillRect can be skipped.

[Pat Galizia]

(In reply to comment #6)
> I think it's OK to make a change like this even without hard performance data. We certainly know that FillRect is slow.
> 
> I would like to see a comment in the code to explain why the fillRect can be skipped.

Sure -- I'll definitely add that for the next patch revision.

[Darin Adler]

The issue is not whether fillRect can be slow. It's whether the cost of checking for this special case pays for itself or not. If the special case is rare enough, the check for it can be more costly than the optimization.

Further, some graphics systems already have early exits for the case of filling objects with a fully-transparent color. So adding a second check of that might help performance on some platforms and hurt performance slightly on others. The better fix if this is the case might be to make sure the lower level graphics code checks for this consistently on all platforms. It's not clear to me that putting this optimization into the rendering layer is the best way to make things faster.

The reason to check at a higher level is that you can save even more computation and overhead if you check at as high a level as possible.

[Simon Fraser (smfr)]

(In reply to comment #8)

> Further, some graphics systems already have early exits for the case of filling objects with a fully-transparent color.

This patch isn't avoiding doing a fillRect for a fully transparent color. It's checking to see if a later fillRect will fully obscure an earlier one, which I think is a fine optimization.

[Darin Adler]

(In reply to comment #9)
> This patch isn't avoiding doing a fillRect for a fully transparent color. It's checking to see if a later fillRect will fully obscure an earlier one, which I think is a fine optimization.

Sorry, I was totally confused. Now that I know what this is, I can make a different comment.

We have tried this optimization multiple times in the past and have never need able to actually squeeze a speed-up out of it. I’m excited that things are different this time, and also a little skeptical because of the previous attempts.

[Darin Adler]

"have never *been* able"

[Pat Galizia]

(In reply to comment #10)
> (In reply to comment #9)
> > This patch isn't avoiding doing a fillRect for a fully transparent color. It's checking to see if a later fillRect will fully obscure an earlier one, which I think is a fine optimization.
> 
> Sorry, I was totally confused. Now that I know what this is, I can make a different comment.
> 
> We have tried this optimization multiple times in the past and have never need able to actually squeeze a speed-up out of it. I’m excited that things are different this time, and also a little skeptical because of the previous attempts.

The improvement may be due to our particular device configuration.  I'll look into applying this to a different device and see if we end up losing cycles with that change there.  If so, we can determine what to do next, since it makes no sense to apply this for one type of device if it regresses others.

[Pat Galizia]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > This patch isn't avoiding doing a fillRect for a fully transparent color. It's checking to see if a later fillRect will fully obscure an earlier one, which I think is a fine optimization.
> > 
> > Sorry, I was totally confused. Now that I know what this is, I can make a different comment.
> > 
> > We have tried this optimization multiple times in the past and have never need able to actually squeeze a speed-up out of it. I’m excited that things are different this time, and also a little skeptical because of the previous attempts.
> 
> The improvement may be due to our particular device configuration.  I'll look into applying this to a different device and see if we end up losing cycles with that change there.  If so, we can determine what to do next, since it makes no sense to apply this for one type of device if it regresses others.

I reran the patch on a higher-end x86_64 Linux system.  I don't see a difference one way or the other at a benchmark level, nor with instrumentation in the code.  From what I'm seeing, it looks like this patch may help lower-end devices, but won't do anything for mid-to-higher-end systems.

[Pat Galizia]

Created attachment 57679 [details]
Added comments to proposed patch. Removed Color.isValid check

Added comments to code explaining why we're making the check.  Also removed the isValid() check, since that wasn't necessary.

Code seems to give a very minor performance boost to small devices, but seems to do nothing for anything above that.  No regressions detected.

[Simon Fraser (smfr)]

Comment on attachment 57679 [details]
Added comments to proposed patch. Removed Color.isValid check

> Index: WebCore/rendering/RenderBoxModelObject.cpp
> ===================================================================

> +                // If the bgColor alpha isn't 255, then we can apply the
> +                // baseColor to fillRect.  If bgColor's alpha is 255, then
> +                // this would be overwritten anyway, so there's not much
> +                // sense in doing it.  It slightly helps performance in
> +                // smaller devices.

The comment is slightly verbose. I think it would be better as:
 // Don't bother filling with the base color if the background color is opaque.

You don't need to explain the performanc implications.

> +                if (bgColor.alpha() != 255) {

Better as

 if (!bgColor.hasAlpha())

> +                    context->save();
> +                    context->setCompositeOperation(CompositeCopy);
> +                    context->fillRect(rect, baseColor, style()->colorSpace());
> +                    context->restore();
> +                }
>              } else
>                  context->clearRect(rect);

Can't you also avoid the clearRect() if the bgColor is opaque?