<rdar://problem/12714586> We're creating/destroying too many CALayers when scrolling. We should recycle tiles.
Adding msaboff since he added MemoryPressureHandler.
Created attachment 176606 [details] Patch
Comment on attachment 176606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176606&action=review > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.h:43 > +class TileLayerPool { I don't think this should be 'TileLayerPool' because we should use it for non-tile layers too. Just LayerPool should be fine. > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.h:55 > + unsigned decayedCapacity() const; What does 'decayed' mean? > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.h:74 > + unsigned m_totalBytes; > + unsigned m_capacity; Would be nice to name m_capacity to indicate what units it's in (bytes?) > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:72 > + IntSize layerSize([layer.get() frame].size); Would be better to use the bounds size (frame is a synthetic property on CALayer). > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:76 > + MutexLocker locker(m_layerPoolMutex); We don't need locking on desktop. You could #ifdef this PLATFORM(IOS) > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:103 > + return m_capacity / 4 + m_capacity * 3 / 4 * (1.f - decayProgess); Don't need 1.f; just 1. > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:115 > + dispatch_time_t nextPruneTime = dispatch_time(DISPATCH_TIME_NOW, 1 * NSEC_PER_SEC); > + dispatch_after(nextPruneTime, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ > + prune(); > + }); Why isn't this just a Timer?
(In reply to comment #3) > (From update of attachment 176606 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176606&action=review > > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.h:43 > > +class TileLayerPool { > > I don't think this should be 'TileLayerPool' because we should use it for non-tile layers too. Just LayerPool should be fine. > Fixed. > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.h:55 > > + unsigned decayedCapacity() const; > > What does 'decayed' mean? > Good question. I don't have a great answer for this though. We should get Antti to weigh in. The goal in general is to shrink the LayerPool over time. If layers have been in there for a long time, then we want to get rid of some of them. So there is capacity and "decayedCapacity." Capacity is the absolute maximum amount of space we want the layers to take up, and decayedCapacity is a lesser number that we will prune to with some regularity. > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.h:74 > > + unsigned m_totalBytes; > > + unsigned m_capacity; > > Would be nice to name m_capacity to indicate what units it's in (bytes?) > I re-named this to m_maxBytesForPool. I'm open to other suggestions too. > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:72 > > + IntSize layerSize([layer.get() frame].size); > > Would be better to use the bounds size (frame is a synthetic property on CALayer). > Fixed. > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:76 > > + MutexLocker locker(m_layerPoolMutex); > > We don't need locking on desktop. You could #ifdef this PLATFORM(IOS) > I removed this. > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:103 > > + return m_capacity / 4 + m_capacity * 3 / 4 * (1.f - decayProgess); > > Don't need 1.f; just 1. > Fixed. > > Source/WebCore/platform/graphics/ca/mac/TileLayerPool.mm:115 > > + dispatch_time_t nextPruneTime = dispatch_time(DISPATCH_TIME_NOW, 1 * NSEC_PER_SEC); > > + dispatch_after(nextPruneTime, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ > > + prune(); > > + }); > > Why isn't this just a Timer? It is now!
Created attachment 176646 [details] Patch
Attachment 176646 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/ca/mac/LayerPool.h:66: The parameter name "accessType" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/graphics/ca/mac/LayerPool.h:68: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/graphics/ca/mac/LayerPool.h:71: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 176772 [details] Patch
Comment on attachment 176772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176772&action=review > Source/WebCore/platform/graphics/ca/mac/LayerPool.h:48 > + void addLayer(const RetainPtr<WebTileLayer>&); > + RetainPtr<WebTileLayer> takeLayerWithSize(const IntSize&); Crap, now I realize that we use a special CALayer subclass for tiles. So maybe this is a TiledLayerPool! But LayerPool is OK for now. Or I wonder if we could avoid using a subclass and make WebLayer more dynamic. > Source/WebCore/platform/graphics/ca/mac/LayerPool.h:54 > + unsigned decayedCapacity() const; Why is this public? > Source/WebCore/platform/graphics/ca/mac/LayerPool.h:68 > + static unsigned bytesBackingLayerWithPixelSize(const IntSize&); Awkward name. Maybe backingStoreBytesForSize()? > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:491 > + RetainPtr<WebTileLayer> layer = LayerPool::sharedPool()->takeLayerWithSize(tileRect.size()); If you get a layer from the pool I think you should -setNeedsDisplay:YES on it to avoid stale content.
Thanks, Simon! I addressed your remaining comments. http://trac.webkit.org/changeset/136151