Bug 100413 - [cg] RenderBlock::selectionGaps() is extremely slow when there are many floats
Summary: [cg] RenderBlock::selectionGaps() is extremely slow when there are many floats
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://code.cafemix.jp/%E3%82%BD%E3%8...
Keywords: InRadar
: 39380 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-10-25 14:11 PDT by mitz
Modified: 2012-11-16 18:30 PST (History)
7 users (show)

See Also:


Attachments
Add GraphicsContext::clipOut(Vector<IntRect>&) and deploy it in RenderBlock::selectionGaps() (10.02 KB, patch)
2012-10-25 14:20 PDT, mitz
adele: review+
Details | Formatted Diff | Diff
Remove an unnecessary call to CGContextGetClipBoundingBox from GraphicsContext::clipOut() (1.64 KB, patch)
2012-10-25 15:12 PDT, mitz
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-10-25 14:11:19 PDT
<rdar://problem/12544626>

To reproduce: navigate to the URL and select one of the category names in red.
Comment 1 mitz 2012-10-25 14:20:36 PDT
Created attachment 170730 [details]
Add GraphicsContext::clipOut(Vector<IntRect>&) and deploy it in RenderBlock::selectionGaps()
Comment 2 Darin Adler 2012-10-25 14:30:03 PDT
Comment on attachment 170730 [details]
Add GraphicsContext::clipOut(Vector<IntRect>&) and deploy it in RenderBlock::selectionGaps()

View in context: https://bugs.webkit.org/attachment.cgi?id=170730&action=review

I’m OK with this, but I suspect there may be two other variations that work equally well or perhaps even better.

> Source/WebCore/ChangeLog:10
> +        RenderBlock::selectionGaps() calls GraphicsContext::clipOut(const IntRect&) for each float.
> +        With Core Graphics, A function that takes a vector of rectangles and clips them all out at
> +        once is faster than multiple calls to the function that clips out a single rectangle.

Is the issue here the performance of repeatedly calling the CGContextGetClipBoundingBox function?

If so, could we fix this performance issue another way by simply using a wide open rectangle instead of calling CGContextGetClipBoundingBox?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1080
> +        CGContextBeginPath(context);
> +        CGContextAddRects(context, cgRects, 2);
> +        CGContextEOClip(context);

Can the calls to CGContextBeginPath and CGContextEOClip be moved outside the loop? I don’t entirely understand the semantics here, but if all the rectangles are exclusive or'ed with each other, it seems that it could all be done with a single path.
Comment 3 mitz 2012-10-25 14:46:56 PDT
(In reply to comment #2)
> (From update of attachment 170730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170730&action=review
> 
> I’m OK with this, but I suspect there may be two other variations that work equally well or perhaps even better.
> 
> > Source/WebCore/ChangeLog:10
> > +        RenderBlock::selectionGaps() calls GraphicsContext::clipOut(const IntRect&) for each float.
> > +        With Core Graphics, A function that takes a vector of rectangles and clips them all out at
> > +        once is faster than multiple calls to the function that clips out a single rectangle.
> 
> Is the issue here the performance of repeatedly calling the CGContextGetClipBoundingBox function?

That’s the most expensive part. There’s also some function call overhead, but I didn’t profile this so I don’t know if it’s significant.

> 
> If so, could we fix this performance issue another way by simply using a wide open rectangle instead of calling CGContextGetClipBoundingBox?

I am going to try that.

> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1080
> > +        CGContextBeginPath(context);
> > +        CGContextAddRects(context, cgRects, 2);
> > +        CGContextEOClip(context);
> 
> Can the calls to CGContextBeginPath and CGContextEOClip be moved outside the loop? I don’t entirely understand the semantics here, but if all the rectangles are exclusive or'ed with each other, it seems that it could all be done with a single path.

We’d need to know that all rects in the input vector are pairwise disjoint.
Comment 4 mitz 2012-10-25 15:12:54 PDT
Created attachment 170741 [details]
Remove an unnecessary call to CGContextGetClipBoundingBox from GraphicsContext::clipOut()
Comment 5 mitz 2012-10-25 17:05:38 PDT
Fixed in <http://trac.webkit.org/r132545>.
Comment 6 Nate Whetsell 2012-11-16 18:30:23 PST
*** Bug 39380 has been marked as a duplicate of this bug. ***