Bug 142417 - [CG] Have Canvas use the IOSurfacePool
Summary: [CG] Have Canvas use the IOSurfacePool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 147494
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-06 16:07 PST by Chris Dumez
Modified: 2015-07-31 11:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.55 KB, patch)
2015-03-06 17:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.78 KB, patch)
2015-03-07 23:13 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (28.98 KB, patch)
2015-03-08 15:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (28.98 KB, patch)
2015-03-08 15:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (1.11 MB, application/zip)
2015-03-08 15:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (1.10 MB, application/zip)
2015-03-08 15:52 PDT, Build Bot
no flags Details
Patch (29.64 KB, patch)
2015-03-08 15:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-03-06 16:07:33 PST
Have Canvas use the IOSurfacePool to avoid repeatedly creating IOSurfaces.

Radar: <rdar://problem/20044440>
Comment 1 Chris Dumez 2015-03-06 17:48:58 PST
Created attachment 248119 [details]
Patch
Comment 2 Tim Horton 2015-03-06 17:59:23 PST
Comment on attachment 248119 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:50
> +    if (cachedSurface->contextSize() != contextSize)

I thought you mentioned that this was slow. Did further testing invalidate that result?

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:162
> +    // Release the graphics context and update the context size. Next time, the graphics context is

no comma between time and the
Comment 3 Chris Dumez 2015-03-06 19:34:28 PST
Comment on attachment 248119 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:50
>> +    if (cachedSurface->contextSize() != contextSize)
> 
> I thought you mentioned that this was slow. Did further testing invalidate that result?

I was wrong, I had a bug and it was causing the slow down.
Comment 4 Chris Dumez 2015-03-07 23:13:51 PST
Created attachment 248180 [details]
Patch
Comment 5 Darin Adler 2015-03-08 10:43:21 PDT
Comment on attachment 248180 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:52
>  class ImageBufferData {
>  public:
>      ImageBufferData();
> +    ~ImageBufferData();

Since all the data members are public, we should change this to a struct and remove all the "m_" prefixes.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:46
> +    std::unique_ptr<IOSurface> cachedSurface = IOSurfacePool::sharedPool().takeSurface(size, colorSpace);

I suggest using auto here instead of std::unique_ptr<IOSurface>.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:51
> +    if (cachedSurface->contextSize() != contextSize)
> +        cachedSurface->setContextSize(contextSize);

Why not put the == check inside setContextSize instead?

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:57
> +    if (std::unique_ptr<IOSurface> cachedSurface = surfaceFromPool(size, size, colorSpace))

I suggest auto here instead of naming the type.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:59
>      return std::unique_ptr<IOSurface>(new IOSurface(size, colorSpace));

I suggest make_unique here.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:64
> +    if (std::unique_ptr<IOSurface> cachedSurface = surfaceFromPool(size, contextSize, colorSpace))

I suggest auto here instead of naming the type.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:66
>      return std::unique_ptr<IOSurface>(new IOSurface(size, contextSize, colorSpace));

I suggest make_unique here.
Comment 6 Chris Dumez 2015-03-08 11:51:15 PDT
Comment on attachment 248180 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:51
>> +        cachedSurface->setContextSize(contextSize);
> 
> Why not put the == check inside setContextSize instead?

The cost of calling the setContextSize() method was showing on the performance test. Since setContextSize() is private, I could also mark it as inline and move the contextSize check to the method if you'd prefer.
Comment 7 Chris Dumez 2015-03-08 14:09:35 PDT
Comment on attachment 248180 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:51
>>> +        cachedSurface->setContextSize(contextSize);
>> 
>> Why not put the == check inside setContextSize instead?
> 
> The cost of calling the setContextSize() method was showing on the performance test. Since setContextSize() is private, I could also mark it as inline and move the contextSize check to the method if you'd prefer.

Never mind, I tested again today and moving the check to setContextSize() did not affect the results on Canvas/reuse.html. I'll make this change.
Comment 8 Chris Dumez 2015-03-08 14:31:56 PDT
Comment on attachment 248180 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:59
>>      return std::unique_ptr<IOSurface>(new IOSurface(size, colorSpace));
> 
> I suggest make_unique here.

In order to use make_unique<>(), I would need to make the constructors public. However, we really don't want people to call these constructors directly as this would bypass the IOSurfacePool.
Comment 9 Chris Dumez 2015-03-08 15:19:46 PDT
Created attachment 248207 [details]
Patch
Comment 10 Chris Dumez 2015-03-08 15:21:57 PDT
Created attachment 248208 [details]
Patch
Comment 11 Build Bot 2015-03-08 15:47:25 PDT
Comment on attachment 248208 [details]
Patch

Attachment 248208 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5132014336344064

Number of test failures exceeded the failure limit.
Comment 12 Build Bot 2015-03-08 15:47:30 PDT
Created attachment 248211 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 13 Build Bot 2015-03-08 15:52:46 PDT
Comment on attachment 248208 [details]
Patch

Attachment 248208 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4850539359633408

Number of test failures exceeded the failure limit.
Comment 14 Build Bot 2015-03-08 15:52:52 PDT
Created attachment 248214 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 15 Chris Dumez 2015-03-08 15:56:58 PDT
Created attachment 248215 [details]
Patch
Comment 16 WebKit Commit Bot 2015-03-09 18:24:43 PDT
Comment on attachment 248215 [details]
Patch

Clearing flags on attachment: 248215

Committed r181301: <http://trac.webkit.org/changeset/181301>
Comment 17 WebKit Commit Bot 2015-03-09 18:24:48 PDT
All reviewed patches have been landed.  Closing bug.