Bug 125477 – Allow ImageBuffer to re-use IOSurfaces

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Myles C. Maxfield]

Allow ImageBuffer to re-use IOSurfaces

[Myles C. Maxfield]

Created attachment 218818 [details]
Patch

[WebKit Commit Bot]

Attachment 218818 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:35:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:113:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

The style errors are caused by bugs in check-webkit-style. I will fix that script tomorrow.

[Build Bot]

Comment on attachment 218818 [details]
Patch

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

[Build Bot]

Comment on attachment 218818 [details]
Patch

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

[Build Bot]

Comment on attachment 218818 [details]
Patch

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

[Tim Horton]

Comment on attachment 218818 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:128
> +        if (info.colorSpace == colorSpace) {

Comparing CGColorSpaceRefs by pointer value?!

[Tim Horton]

Comment on attachment 218818 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:255
> +    CGContextClearRect(context, CGRectMake(0, 0, IOSurfaceGetWidth(ioSurface), IOSurfaceGetHeight(ioSurface)));

two calls to getwidth and getheight in this function.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:86
> +    ActiveIOSurfaceLookupMap m_activeIOSurfaces;

What's the point? I can't figure out the reason for keeping track of active surfaces.

> LayoutTests/fast/canvas/canvas-backing-store-reuse.html:14
> +  canvas.height = 100;

This test seems weird. The first canvas is smaller than the second one? Won't that mean that the second one necessarily never reuses the IOSurface from the first one?
What ensures that we've flushed before throwing away the first canvas? etc.

[Myles C. Maxfield]

Comment on attachment 218818 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:128
>> +        if (info.colorSpace == colorSpace) {
> 
> Comparing CGColorSpaceRefs by pointer value?!

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:255
>> +    CGContextClearRect(context, CGRectMake(0, 0, IOSurfaceGetWidth(ioSurface), IOSurfaceGetHeight(ioSurface)));
> 
> two calls to getwidth and getheight in this function.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:86
>> +    ActiveIOSurfaceLookupMap m_activeIOSurfaces;
> 
> What's the point? I can't figure out the reason for keeping track of active surfaces.

To keep track of the CGContext and CGColorSpaceRef along with the IOSurface. I only want clients to have to hand back a single key, the IOSurfaceRef, so I have to have some way of looking up this other information from just this key. We need to remember what the color space to see if we're able to re-use it later.

>> LayoutTests/fast/canvas/canvas-backing-store-reuse.html:14
>> +  canvas.height = 100;
> 
> This test seems weird. The first canvas is smaller than the second one? Won't that mean that the second one necessarily never reuses the IOSurface from the first one?
> What ensures that we've flushed before throwing away the first canvas? etc.

Not at all - ImageBuffers can use larger-than-necessary IOSurfaces since r160121, and will do so if we find a cached IOSurface that is greater than the size we're looking for. However, a client can specify that they are only willing to accept a perfectly sized IOSurface using the needExactSize argument to ImageBufferBackingStoreCache::getIOSurface().

Flushing itself shouldn't be necessary; instead I clear the entire IOSurface in ImageBufferBackingStoreCache::releaseIOSurface().

[Myles C. Maxfield]

Style fixes are in https://bugs.webkit.org/show_bug.cgi?id=125534 and https://bugs.webkit.org/show_bug.cgi?id=125537

[Myles C. Maxfield]

Created attachment 218899 [details]
Patch

[WebKit Commit Bot]

Attachment 218899 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:42:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:119:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 218899 [details]
Patch

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

[Build Bot]

Comment on attachment 218899 [details]
Patch

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

[Sam Weinig]

Comment on attachment 218899 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:70
> +    typedef std::unordered_map<IOSurfaceRef, InternalIOSurfaceInfo> ActiveIOSurfaceLookupMap;
> +    typedef std::unordered_map<IOSurfaceRef, CachedIOSurfaceInfo> CachedIOSurfaceLookupMap;
> +    typedef std::multimap<int, CachedIOSurfaceLookupMap::iterator> DimensionalIOSurfaceLookupMap;

We tend to not use standard library containers.  Please convert to using WTF versions.

[Myles C. Maxfield]

Comment on attachment 218899 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:70
>> +    typedef std::multimap<int, CachedIOSurfaceLookupMap::iterator> DimensionalIOSurfaceLookupMap;
> 
> We tend to not use standard library containers.  Please convert to using WTF versions.

I can't do that for a couple reasons:
1) I need a multimap which is ordered, because I iterate over the keys and I expect them to be in order when I do so. If we had an ordered map I could use a map of vectors to approximate a multimap; however, it appears that we don't have any sort of ordered data structure other than a vector (which is a no-go due to the runtime of insertion and removal)
2) I can't even use WTF::HashMap for the unordered_map here because this patch requires the ability to have multiple iterators pointing inside the same map, which WTF::HashTable doesn't allow. Indeed, HashTable::add() calls invalidateIterators() explicitly (HashTable.h:801)

I should, however, be able to make my CandidateMap a HashMap.

[Myles C. Maxfield]

Created attachment 218995 [details]
Patch

[WebKit Commit Bot]

Attachment 218995 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:42:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:119:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 218995 [details]
Patch

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

[Myles C. Maxfield]

Looks like the Mac bots are still on 10.8, and the Purgeable calls I'm making are in 10.9 only

[Build Bot]

Comment on attachment 218995 [details]
Patch

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

[Myles C. Maxfield]

Created attachment 219012 [details]
Patch

[WebKit Commit Bot]

Attachment 219012 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:43:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Comment on attachment 218899 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:70
>>> +    typedef std::multimap<int, CachedIOSurfaceLookupMap::iterator> DimensionalIOSurfaceLookupMap;
>> 
>> We tend to not use standard library containers.  Please convert to using WTF versions.
> 
> I can't do that for a couple reasons:
> 1) I need a multimap which is ordered, because I iterate over the keys and I expect them to be in order when I do so. If we had an ordered map I could use a map of vectors to approximate a multimap; however, it appears that we don't have any sort of ordered data structure other than a vector (which is a no-go due to the runtime of insertion and removal)
> 2) I can't even use WTF::HashMap for the unordered_map here because this patch requires the ability to have multiple iterators pointing inside the same map, which WTF::HashTable doesn't allow. Indeed, HashTable::add() calls invalidateIterators() explicitly (HashTable.h:801)
> 
> I should, however, be able to make my CandidateMap a HashMap.

***requires the ability to have persistent iterators

[Build Bot]

Comment on attachment 219012 [details]
Patch

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

[Myles C. Maxfield]

Created attachment 219018 [details]
Patch

[WebKit Commit Bot]

Attachment 219018 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:44:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 219018 [details]
Patch

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

[Build Bot]

Comment on attachment 219018 [details]
Patch

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

[Myles C. Maxfield]

Created attachment 219045 [details]
Patch

[Build Bot]

Comment on attachment 219045 [details]
Patch

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

[Build Bot]

Comment on attachment 219045 [details]
Patch

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

[Myles C. Maxfield]

Created attachment 219054 [details]
Patch

[Build Bot]

Comment on attachment 219054 [details]
Patch

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

New failing tests:
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 219060 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219054 [details]
Patch

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

New failing tests:
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 219066 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219054 [details]
Patch

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

New failing tests:
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 219071 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

Created attachment 219213 [details]
Patch

[Myles C. Maxfield]

Breaking out the multimap piece into https://bugs.webkit.org/show_bug.cgi?id=125827

[Geoffrey Garen]

Comment on attachment 219213 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:236
> +    while (xIterator != m_xDimensionLookupMap.end() || yIterator != m_yDimensionLookupMap.end()) {
> +        ret = findIOSurfaceInfoHelper<0>(xIterator, m_xDimensionLookupMap, candidates, size, colorSpace, needExactSize);
> +        if (ret.success && (bestEver.iter == m_cachedIOSurfaces.end()
> +            || betterSurface(size, ret.iter->value, bestEver.iter->value, stopSearching)))
> +            bestEver = ret;
> +        if (stopSearching)
> +            break;
> +        ret = findIOSurfaceInfoHelper<1>(yIterator, m_yDimensionLookupMap, candidates, size, colorSpace, needExactSize);
> +        if (ret.success && (bestEver.iter == m_cachedIOSurfaces.end()
> +            || betterSurface(size, ret.iter->value, bestEver.iter->value, stopSearching)))
> +            bestEver = ret;
> +        if (stopSearching)
> +            break;
> +    }

If we have lots of surfaces with the same width and/or height, this code is O(N). That case seems likely to me. I don't think this much complexity is justified just to achieve O(N).

[Geoffrey Garen]

How many IOSurfaces do you plan to cache?

(1) If the answer is < 50, then I suggest a simple vector or doubly-linked list with linear search.

The Big-O complexity of linear search is just as good as the algorithm you've posted here, and the code complexity is about ten orders of magnitude lower -- which is, if you do the math, an order of orders of magnitude.

I believe you can do this in about 10 lines of code.

(2) If the answer is >= 50, then I suggest the following:

(1) Create a hash table in which the key is an area (width x height), rounded down to the nearest power of two (which is a simple mask instruction), and the value is a deque.

(2) Lookup should select a deque, and iterate end to start (LIFO order), to find an appropriate candidate. The common case will take the very last item, which is both the last item inserted and therefore cache-local, and also constant-time to remove.

(3) Insertion should select a deque and add to the end. If the deque exceeds a fixed limit in size, insertion should also remove from the beginning of the deque (FIFO order).

I believe you can do this in about 20 lines of code.

[Myles C. Maxfield]

Created attachment 219628 [details]
Patch

[Myles C. Maxfield]

I'll fix check-webkit-style tomorrow regarding the style errors.

[WebKit Commit Bot]

Attachment 219628 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:109:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:72:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 219628 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:23
> + * Copyright (C) 2013 Apple, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 

This is not the right copyright and license header.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:24
> + * Copyright (C) 2013 Apple, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> + */

This is not the right copyright and license header.

[Geoffrey Garen]

Comment on attachment 219628 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:36
> +static const double purgeIntervalInMS = 30000;

This would make more sense specified in seconds.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:98
> +    IntSize actualSize = IntSize(IOSurfaceGetWidth(surface), IOSurfaceGetHeight(surface));

No need for "= IntSize" in this declaration.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:167
> +    InternalIOSurfaceInfo* toAdd = new InternalIOSurfaceInfo(lookup->value);

Manual new/delete is the enemy. You are aiding the enemy.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:168
> +    auto insertedTuple = m_cachedSurfaces.add(convertSizeToKey(surfaceSize), InfoLinkedList());

Should there be a limit on this cache? What happens if I allocate and free lots of canvases in a loop, all with different sizes?

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:69
> +    class InternalIOSurfaceInfo : public DoublyLinkedListNode<InternalIOSurfaceInfo> {

"internal" doesn't really mean anything. Think about what's different between this and IOSurfaceInfo, and put that in the name.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:88
> +    typedef HashMap<IOSurfaceRef, InternalIOSurfaceInfo> ActiveSurfaceMap;

Why is it OK for this to be a raw pointer? Who owns the IOSurfaceRef? I couldn't tell just by skimming the code.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:95
> +        return std::make_pair((size.width() + 7) & ~0x7, (size.height() + 7) & ~0x7);

Please use WTF::roundUpToMultipleOf.

[Myles C. Maxfield]

Comment on attachment 219628 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:23
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> 
> This is not the right copyright and license header.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:36
>> +static const double purgeIntervalInMS = 30000;
> 
> This would make more sense specified in seconds.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:98
>> +    IntSize actualSize = IntSize(IOSurfaceGetWidth(surface), IOSurfaceGetHeight(surface));
> 
> No need for "= IntSize" in this declaration.

This is a holdover from when I was working on Chrome (I got yelled at for not using the assignment operator). Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:167
>> +    InternalIOSurfaceInfo* toAdd = new InternalIOSurfaceInfo(lookup->value);
> 
> Manual new/delete is the enemy. You are aiding the enemy.

Is there a way to use DoublyLinkedListNode<> without manual new + delete? I'm trying to not invent a data structure

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:168
>> +    auto insertedTuple = m_cachedSurfaces.add(convertSizeToKey(surfaceSize), InfoLinkedList());
> 
> Should there be a limit on this cache? What happens if I allocate and free lots of canvases in a loop, all with different sizes?

Straightforward enough. Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:24
>> + */
> 
> This is not the right copyright and license header.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:69
>> +    class InternalIOSurfaceInfo : public DoublyLinkedListNode<InternalIOSurfaceInfo> {
> 
> "internal" doesn't really mean anything. Think about what's different between this and IOSurfaceInfo, and put that in the name.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:88
>> +    typedef HashMap<IOSurfaceRef, InternalIOSurfaceInfo> ActiveSurfaceMap;
> 
> Why is it OK for this to be a raw pointer? Who owns the IOSurfaceRef? I couldn't tell just by skimming the code.

The pointer is retained inside the InternalIOSurfaceInfo (soon to be IOSurfaceInfoWithCreationParams) which is the value type of the map. Originally I didn't think that I could make a map keyed off of a RetainPtr, but it looks like I can. I'll do that.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:95
>> +        return std::make_pair((size.width() + 7) & ~0x7, (size.height() + 7) & ~0x7);
> 
> Please use WTF::roundUpToMultipleOf.

Done.

[Myles C. Maxfield]

Created attachment 219693 [details]
Patch

[Geoffrey Garen]

Comment on attachment 219693 [details]
Patch

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

What I would suggest for eviction:

(1) If new item bigger than cache, do not cache it.

(2) while cache size + new item > limit, evict oldest item in current list.

This is good because old items of similar size, which did not match our query, are probably slightly too small or in the wrong color space.

(3) while cache size + new item > limit, for each bucket, while cache size + new item > limit, evict oldest item in bucket.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:51
> +    const void* keys[6];

Let's pull out the 6 here and make it a local constant.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:106
> +auto ImageBufferBackingStoreCache::findAcceptableCachedSurface(const IntSize& size, CGColorSpaceModel colorSpace, bool needExactSize) -> IOSurfaceInfoWithCreationParams*

Let's make this return a reference to the list plus a pointer.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:115
> +            ll.remove(info);
> +            return info;

Let's factor this into a helper function to do the memory management.

Empty lists also need to be removed from the cache table. Perhaps you can factor this into the helper function.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:129
> +        IntSize surfaceSize = IntSize(IOSurfaceGetWidth(surface), IOSurfaceGetHeight(surface));

"= IntSize".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:133
> +        delete foundPtr;

Helper function to do memory management.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:163
> +        m_activeSurfaces.remove(lookup);

Let's remove old items instead of the new item. Otherwise, the cache will fill up with garbage and stay that way.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:176
> +    auto* toAdd = new IOSurfaceInfoWithCreationParams(lookup->value);
> +    auto insertedTuple = m_cachedSurfaces.add(convertSizeToKey(surfaceSize), InfoLinkedList());
> +    insertedTuple.iterator->value.append(toAdd);

Let's use a helper here too, to which we pass a list reference and lookup->value.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:188
> +            delete ll.removeHead();

Helper function to do memory management.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:198
> +    static const double purgeInterval = 30;

Since the current version of the cache does not make surfaces volatile, and they are wired memory, let's reduce this number to 5s. It can go back up when we do the volatile thing, since we won't monopolize wired memory then.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:50
> +        }

Need a newline after this function.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:107
> +    int pixelsCached;

Please use the "m_" prefix.

[Myles C. Maxfield]

Comment on attachment 219693 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:51
>> +    const void* keys[6];
> 
> Let's pull out the 6 here and make it a local constant.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:106
>> +auto ImageBufferBackingStoreCache::findAcceptableCachedSurface(const IntSize& size, CGColorSpaceModel colorSpace, bool needExactSize) -> IOSurfaceInfoWithCreationParams*
> 
> Let's make this return a reference to the list plus a pointer.

Instead, I'll make this function call the deletion helper directly, instead of passing the arguments to the deletion helper to getIOSurface() so getIOSurface() can call the deletion helper.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:115
>> +            return info;
> 
> Let's factor this into a helper function to do the memory management.
> 
> Empty lists also need to be removed from the cache table. Perhaps you can factor this into the helper function.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:129
>> +        IntSize surfaceSize = IntSize(IOSurfaceGetWidth(surface), IOSurfaceGetHeight(surface));
> 
> "= IntSize".

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:133
>> +        delete foundPtr;
> 
> Helper function to do memory management.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:163
>> +        m_activeSurfaces.remove(lookup);
> 
> Let's remove old items instead of the new item. Otherwise, the cache will fill up with garbage and stay that way.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:176
>> +    insertedTuple.iterator->value.append(toAdd);
> 
> Let's use a helper here too, to which we pass a list reference and lookup->value.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:188
>> +            delete ll.removeHead();
> 
> Helper function to do memory management.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:198
>> +    static const double purgeInterval = 30;
> 
> Since the current version of the cache does not make surfaces volatile, and they are wired memory, let's reduce this number to 5s. It can go back up when we do the volatile thing, since we won't monopolize wired memory then.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:50
>> +        }
> 
> Need a newline after this function.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:107
>> +    int pixelsCached;
> 
> Please use the "m_" prefix.

Done.

[Geoffrey Garen]

Comment on attachment 219693 [details]
Patch

Getting this old patch out of the queue.

[Myles C. Maxfield]

Created attachment 219712 [details]
Patch

[Myles C. Maxfield]

Style is a false negative. Will make a subsequent patch to address.

[WebKit Commit Bot]

Attachment 219712 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/canvas-backing-store-reuse-expected.txt', u'LayoutTests/fast/canvas/canvas-backing-store-reuse.html', u'PerformanceTests/Canvas/reuse.html', u'PerformanceTests/ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp', u'Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h', u'Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:131:  Declaration has space between type name and * in *didRemoveLastElementInList  [whitespace/declaration] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 219712 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:67
> +    RetainPtr<CFDictionaryRef> dict = adoptCF(CFDictionaryCreate(0, keys, values, 6, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

kNumCreationParameters

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:68
> +    for (unsigned i = 0; i < 6; i++)

kNumCreationParameters

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:86
> +ImageBufferBackingStoreCache::IOSurfaceInfoWithCreationParams::IOSurfaceInfoWithCreationParams(IOSurfaceInfo&& info, CGColorSpaceModel colorSpace)
> +    : m_prev(nullptr)
> +    , m_next(nullptr)
> +    , m_info(std::forward<IOSurfaceInfo>(info))
> +    , m_colorSpace(colorSpace)
> +{
> +}

This doesn't look like the right way to do the C++ move idiom.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:112
> +    auto* toAdd = new IOSurfaceInfoWithCreationParams(info);

Should be "auto".

It's not good to take a const& and pass it to a constructor intended to do a move operation. "const&" means "don't modify my data" and "move" means "take all my data away".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:133
> +    if (didRemoveLastElementInList)
> +        *didRemoveLastElementInList = iter->value.isEmpty();
> +    else
> +        m_cachedSurfaces.remove(iter);

This out parameter is pretty awkward. Can you arrange for the delete function to remove the empty list? That's part of the job of having a helper function -- we want a single place to manage the memory.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:137
> +bool ImageBufferBackingStoreCache::findAcceptableCachedSurface(const IntSize& size, CGColorSpaceModel colorSpace, bool needExactSize, IOSurfaceInfoWithCreationParams& outInfo)

I still think this should be called "take", since it removes an item from the cache.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:145
> +            outInfo = deleteFromCache(i, info, nullptr);

I would call this "take". A C++ programmer should instinctively fear and loathe any line of code that says "x = delete y".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:200
> +    // Evict

This code right below this comment doesn't do what this comment says.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:224
> +    Vector<CachedSurfaceKey> bucketsToRemove;
> +    bucket = m_cachedSurfaces.begin();
> +    while (m_pixelsCached + surfaceArea > kMaxPixelsCached) {
> +        bool shouldRemoveEmptyEntry;
> +        deleteFromCache(bucket, bucket->value.head(), &shouldRemoveEmptyEntry);
> +        if (shouldRemoveEmptyEntry)
> +            bucketsToRemove.append(bucket->key);
> +        ++bucket;
> +        if (bucket == m_cachedSurfaces.end()) {
> +            clearEmptyBuckets(bucketsToRemove);
> +            bucket = m_cachedSurfaces.begin();
> +        }
> +    }
> +    clearEmptyBuckets(bucketsToRemove);

This is too much code and CPU time for a not-very-good eviction algorithm. Let's at least get the benefits of a not-very-good eviction algorithm, and keep things simple.

Let's go with removing begin().head() repeatedly.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:62
> +    IOSurfaceInfo getIOSurface(const IntSize&, RetainPtr<CGColorSpaceRef>, bool needExactSize);

I'd call this "getOrAllocate".

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:63
> +    void releaseIOSurface(RetainPtr<IOSurfaceRef>);

I would call this "deallocate". No need to mention IOSurface in the name thrice -- it's already in the argument signature part of the name, and in the class name. "deallocate" helps indicate that things not deallocated will cause memory leaks in the cache. It's important to call that out because most caches don't work that way.

[Simon Fraser (smfr)]

Comment on attachment 219712 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:47
> +    struct IOSurfaceInfo {

"info" is a weird name for something that owns the actual data. It sounds more like a metadata container.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:52
> +        IOSurfaceInfo(RetainPtr<IOSurfaceRef> surface, RetainPtr<CGContextRef> context)

Odd to pass RetainPtrs here.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:91
> +        CGColorSpaceModel m_colorSpace;

You need to hold onto a CGColorSpaceRef, not just the model. But why do you need to retain this and the CGContextRef?

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:94
> +    typedef std::pair<int, int> CachedSurfaceKey;

This could be an IntSize.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:101
> +    CachedSurfaceKey convertSizeToKey(const IntSize& size)
> +    {
> +        return std::make_pair(WTF::roundUpToMultipleOf(8, size.width()), WTF::roundUpToMultipleOf(8, size.height()));
> +    }

static?

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:115
> +    bool findAcceptableCachedSurface(const IntSize&, CGColorSpaceModel, bool needExactSize, IOSurfaceInfoWithCreationParams& outInfo);
> +    bool isAcceptableSurface(const IOSurfaceInfoWithCreationParams&, const IntSize&, CGColorSpaceModel, bool needExactSize);

const

[Myles C. Maxfield]

Comment on attachment 219712 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:67
>> +    RetainPtr<CFDictionaryRef> dict = adoptCF(CFDictionaryCreate(0, keys, values, 6, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> 
> kNumCreationParameters

Oops. Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:68
>> +    for (unsigned i = 0; i < 6; i++)
> 
> kNumCreationParameters

Oops. Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:86
>> +}
> 
> This doesn't look like the right way to do the C++ move idiom.

It isn't intended to be a move constructor. I've removed the && to make this more straightforward.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:112
>> +    auto* toAdd = new IOSurfaceInfoWithCreationParams(info);
> 
> Should be "auto".
> 
> It's not good to take a const& and pass it to a constructor intended to do a move operation. "const&" means "don't modify my data" and "move" means "take all my data away".

My original intention was to copy the object, but moving the object would be better here.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:133
>> +        m_cachedSurfaces.remove(iter);
> 
> This out parameter is pretty awkward. Can you arrange for the delete function to remove the empty list? That's part of the job of having a helper function -- we want a single place to manage the memory.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:137
>> +bool ImageBufferBackingStoreCache::findAcceptableCachedSurface(const IntSize& size, CGColorSpaceModel colorSpace, bool needExactSize, IOSurfaceInfoWithCreationParams& outInfo)
> 
> I still think this should be called "take", since it removes an item from the cache.

Done

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:145
>> +            outInfo = deleteFromCache(i, info, nullptr);
> 
> I would call this "take". A C++ programmer should instinctively fear and loathe any line of code that says "x = delete y".

After inspection, there is no reason why deleteFromCache has to return a value; the caller already has a pointer to the object being deleted. I'll make it a void function instead, and keep its name.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:200
>> +    // Evict
> 
> This code right below this comment doesn't do what this comment says.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:224
>> +    clearEmptyBuckets(bucketsToRemove);
> 
> This is too much code and CPU time for a not-very-good eviction algorithm. Let's at least get the benefits of a not-very-good eviction algorithm, and keep things simple.
> 
> Let's go with removing begin().head() repeatedly.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:47
>> +    struct IOSurfaceInfo {
> 
> "info" is a weird name for something that owns the actual data. It sounds more like a metadata container.

I've renamed this to IOSurfaceAndContext, and I've renamed IOSurfaceInfoWithCreationParams accordingly.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:52
>> +        IOSurfaceInfo(RetainPtr<IOSurfaceRef> surface, RetainPtr<CGContextRef> context)
> 
> Odd to pass RetainPtrs here.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:62
>> +    IOSurfaceInfo getIOSurface(const IntSize&, RetainPtr<CGColorSpaceRef>, bool needExactSize);
> 
> I'd call this "getOrAllocate".

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:63
>> +    void releaseIOSurface(RetainPtr<IOSurfaceRef>);
> 
> I would call this "deallocate". No need to mention IOSurface in the name thrice -- it's already in the argument signature part of the name, and in the class name. "deallocate" helps indicate that things not deallocated will cause memory leaks in the cache. It's important to call that out because most caches don't work that way.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:91
>> +        CGColorSpaceModel m_colorSpace;
> 
> You need to hold onto a CGColorSpaceRef, not just the model. But why do you need to retain this and the CGContextRef?

Done. I hold on to it so I can see if an item in the cache matches the creation parameters that the client asks for.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:94
>> +    typedef std::pair<int, int> CachedSurfaceKey;
> 
> This could be an IntSize.

the hash function isn't declared for IntSizes

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:101
>> +    }
> 
> static?

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:115
>> +    bool isAcceptableSurface(const IOSurfaceInfoWithCreationParams&, const IntSize&, CGColorSpaceModel, bool needExactSize);
> 
> const

Done.

[Myles C. Maxfield]

Created attachment 219732 [details]
Patch

[Geoffrey Garen]

Comment on attachment 219732 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:78
> +bool ImageBufferBackingStoreCache::isAcceptableSurface(IOSurfaceAndContextWithCreationParams& info, IntSize& requestedSize, CGColorSpaceRef colorSpace, bool needExactSize)

These arguments should be const&.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:126
> +            outInfo = std::move(*info);
> +            deleteFromCache(i, info);

This is basically use after free, since deletedFromCache will use info after it has been moved.

[Myles C. Maxfield]

Comment on attachment 219732 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:78
>> +bool ImageBufferBackingStoreCache::isAcceptableSurface(IOSurfaceAndContextWithCreationParams& info, IntSize& requestedSize, CGColorSpaceRef colorSpace, bool needExactSize)
> 
> These arguments should be const&.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:126
>> +            deleteFromCache(i, info);
> 
> This is basically use after free, since deletedFromCache will use info after it has been moved.

Done.

[Myles C. Maxfield]

Created attachment 219792 [details]
Patch

[Geoffrey Garen]

Comment on attachment 219792 [details]
Patch

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

r=me

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:116
> +    return result;

You gotta move result, too, or no optimization is guaranteed.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:171
> +    IntSize surfaceSize = IntSize(IOSurfaceGetWidth(ioSurface), IOSurfaceGetHeight(ioSurface));

"= IntSize"

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:192
> +    // Clear opportunistically so CG has more time to carry it out

Let's put a period at the end of this, to make it a sentence.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:101
> +    // If we find an acceptable surface, this function removes it from the cache as
> +    // well as placing it in the out parameter

Period at end of sentence, please.

> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:102
> +    bool tryTakeAcceptableCachedSurface(const IntSize&, CGColorSpaceRef, bool needExactSize, IOSurfaceAndContextWithCreationParams& outInfo);

Let's call this tryTakeFromCache, since it's a wrapper around takeFromCache.

> PerformanceTests/Canvas/reuse.html:7
> +var numCreated = 10000;

The period of this benchmark seems so big that no reasonably-sized cache will ever get a hit. So, this benchmark is artificially biased toward giant caches, and it doesn't test much in the way of eviction algorithm.

Maybe just change it to fewer canvases.

[Myles C. Maxfield]

Comment on attachment 219792 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:116
>> +    return result;
> 
> You gotta move result, too, or no optimization is guaranteed.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:171
>> +    IntSize surfaceSize = IntSize(IOSurfaceGetWidth(ioSurface), IOSurfaceGetHeight(ioSurface));
> 
> "= IntSize"

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.cpp:192
>> +    // Clear opportunistically so CG has more time to carry it out
> 
> Let's put a period at the end of this, to make it a sentence.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:101
>> +    // well as placing it in the out parameter
> 
> Period at end of sentence, please.

Done.

>> Source/WebCore/platform/graphics/cg/ImageBufferBackingStoreCache.h:102
>> +    bool tryTakeAcceptableCachedSurface(const IntSize&, CGColorSpaceRef, bool needExactSize, IOSurfaceAndContextWithCreationParams& outInfo);
> 
> Let's call this tryTakeFromCache, since it's a wrapper around takeFromCache.

Done.

>> PerformanceTests/Canvas/reuse.html:7
>> +var numCreated = 10000;
> 
> The period of this benchmark seems so big that no reasonably-sized cache will ever get a hit. So, this benchmark is artificially biased toward giant caches, and it doesn't test much in the way of eviction algorithm.
> 
> Maybe just change it to fewer canvases.

Done.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126164

[Alexey Proskuryakov]

Rolled out in <https://trac.webkit.org/r161001>. This seems to have made multiple canvas tests flaky:

fast/canvas/canvas-to-canvas.html
fast/canvas/canvas-style-intact-after-text.html
fast/canvas/canvas-strokeText-invalid-maxWidth.html
...and more

I couldn't reproduce the flakiness locally. Will roll back in if this doesn't help tests.

[Alexey Proskuryakov]

Comment on attachment 219792 [details]
Patch

The rollout did fix the tests, so removing r+ flag from the latest patch.

[Tim Horton]

(In reply to comment #67)
> Rolled out in <https://trac.webkit.org/r161001>. This seems to have made multiple canvas tests flaky:
> 
> fast/canvas/canvas-to-canvas.html
> fast/canvas/canvas-style-intact-after-text.html
> fast/canvas/canvas-strokeText-invalid-maxWidth.html
> ...and more
> 
> I couldn't reproduce the flakiness locally. Will roll back in if this doesn't help tests.

On both Mavericks and Mountain Lion, and both WebKit1 and WebKit2, it looks like?

I can't reproduce either :(

[Myles C. Maxfield]

I have tried running tests again on the same hardware as the bots, but cannot reproduce there either

[Jon Lee]

(In reply to comment #70)
> I have tried running tests again on the same hardware as the bots, but cannot reproduce there either

Is it possible because the bots are not attached to screens?

[Myles C. Maxfield]

The hardware I tested on was not attached to a screen.

[Ryosuke Niwa]

You need to boot the machine in the state where display cable is not connected.  Disconnecting a display after the fact has no effect.

[Ryosuke Niwa]

Also... you can't have VNC, etc... connected to the machine.  The machine needs to be in a state where there has been no visual output since it had booted.

[Myles C. Maxfield]

rniwa: It was in that state. lforschler had an idea to test if the machine not having a monitor had anything to do with it, so we devised a test :)

[Myles C. Maxfield]

Tim has taken this over.