Bug 103577 – TiledDrawingArea should recycle tiles

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

<rdar://problem/12714586>

We're creating/destroying too many CALayers when scrolling. We should recycle tiles.

[Beth Dakin]

Adding msaboff since he added MemoryPressureHandler.

[Beth Dakin]

Created attachment 176606 [details]
Patch

[Simon Fraser (smfr)]

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?

[Beth Dakin]

(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!

[Beth Dakin]

Created attachment 176646 [details]
Patch

[WebKit Review Bot]

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.

[Beth Dakin]

Created attachment 176772 [details]
Patch

[Simon Fraser (smfr)]

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.

[Beth Dakin]

Thanks, Simon! I addressed your remaining comments. http://trac.webkit.org/changeset/136151