Bug 100413

Summary: [cg] RenderBlock::selectionGaps() is extremely slow when there are many floats
Product: WebKit Reporter: mitz
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, d-r, eric, nathan.whetsell, noam, senorblanco, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://code.cafemix.jp/%E3%82%BD%E3%83%95%E3%83%88%E3%82%A6%E3%82%A7%E3%82%A2/%E3%83%A6%E3%83%BC%E3%82%B6%E3%81%8C%E7%B7%A8%E9%9B%86%E3%82%82%E3%81%A7%E3%81%8D%E3%82%8B%E3%82%A4%E3%83%B3%E3%82%BF%E3%83%A9%E3%82%AF%E3%83%86%E3%82%A3%E3%83%96%E3%81%AAjavascript%E3%83%87%E3%83%A2/
Attachments:
Description Flags
Add GraphicsContext::clipOut(Vector<IntRect>&) and deploy it in RenderBlock::selectionGaps()
adele: review+
Remove an unnecessary call to CGContextGetClipBoundingBox from GraphicsContext::clipOut() andersca: review+

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. ***