Bug 93305 – Add optional debug logging for tiled scrolling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Horton]

It'd be nice if we could log when painting can't keep up with tiled scrolling (i.e. the "white flash" (or black, as the case may be) that people see when they scroll too fast).

[Tim Horton]

This can't use a log channel because it needs to work in release mode.

[Tim Horton]

Created attachment 157297 [details]
patch

[Tim Horton]

Created attachment 157298 [details]
style

[Simon Fraser (smfr)]

Comment on attachment 157298 [details]
style

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:299
> +void ScrollingTreeNodeMac::logExposedUnfilledArea()

This needs a big scarey comment (or an assertion) about being called on the scrolling thread.

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:301
> +    Region paintedTileCoveredVisibleRegion;

Too many adjectives!

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:310
> +        NSArray* sublayers = [[layer sublayers] copy];

You're leaking the array.

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:312
> +        for (CALayer* sublayer in sublayers)
> +            layerQueue.append(sublayer);

Not sure why you descend into sublayers here like this; can't you assume something about layer tree structure?

> Source/WebCore/platform/graphics/TiledBacking.h:45
> +    bool scrollingPerformanceLoggingEnabled() { return m_scrollingPerformanceLoggingEnabled; }

const please.

> Source/WebCore/platform/graphics/ca/mac/TileCache.h:72
> +    IntRect visibleRect() { return m_visibleRect; }
> +    unsigned blankPixelCount();

const please.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:320
> +        RetainPtr<WebTileLayer>& tileLayer = it->second;

Probably simpler as WebTileLayer* tileLayer = it->second.get()

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:332
> +        IntRect visiblePart([tileLayer.get() frame]);
> +        visiblePart.intersect(m_visibleRect);
> +
> +        if(!visiblePart.isEmpty() && [tileLayer.get() repaintCount])
> +            paintedTileCoveredVisibleRegion.unite(visiblePart);
> +    }
> +
> +    Region uncoveredRegion(m_visibleRect);
> +    uncoveredRegion.subtract(paintedTileCoveredVisibleRegion);
> +
> +    return uncoveredRegion.totalArea();

This seems similar to the logExposedUnfilledArea() code. Why do you need both?

missing space after if

> Source/WebCore/platform/graphics/ca/mac/WebTileLayer.mm:68
> +- (unsigned)repaintCount
> +{
> +    return _repaintCount;
> +}

Do we waste cycles maintaining _repaintCount even when not showing counters?

> Source/WebCore/platform/graphics/ca/mac/WebTileLayer.mm:75
> +    if([self repaintCount] == 1 && !visiblePart.isEmpty())

Space after if

[Tim Horton]

Created attachment 157321 [details]
revised patch

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 157298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157298&action=review
> 
> > Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:299
> > +void ScrollingTreeNodeMac::logExposedUnfilledArea()
> 
> This needs a big scarey comment (or an assertion) about being called on the scrolling thread.

That whole file lives on the scrolling thread, as far as I can tell. It seems like this would be a bit out of place.

> Not sure why you descend into sublayers here like this; can't you assume something about layer tree structure?

The new patch assumes more, and won't crawl the whole tree, but I'm still not sure how to get straight to the tiles.

> This seems similar to the logExposedUnfilledArea() code. Why do you need both?

Fixed.

> Do we waste cycles maintaining _repaintCount even when not showing counters?

We do! We should file a bug, since that behavior already existed, and we should only maintain _repaintCount when scrolling performance logging or layer repaint counters are enabled.

[Build Bot]

Comment on attachment 157321 [details]
revised patch

Attachment 157321 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13462323

[Tim Horton]

Created attachment 157362 [details]
humorously, fix mac build

[Tim Horton]

Created attachment 157368 [details]
negate the offset like it used to be

[Tim Horton]

Created attachment 157373 [details]
typo'd 'layer' for 'sublayer'

[Simon Fraser (smfr)]

Comment on attachment 157368 [details]
negate the offset like it used to be

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:319
> +            break;

When you break here you still leak sublayers.

[Tim Horton]

(In reply to comment #11)
> (From update of attachment 157368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157368&action=review
> 
> > Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:319
> > +            break;
> 
> When you break here you still leak sublayers.

I caught that in my later patch (though failed to note that in my comment, hoping nobody would notice that patch :D).

[Simon Fraser (smfr)]

Comment on attachment 157373 [details]
typo'd 'layer' for 'sublayer'

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

> Source/WebCore/platform/graphics/ca/mac/TileCache.h:71
> +    IntRect visibleRect() { return m_visibleRect; }

const

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:317
> +    Vector<RetainPtr<WebTileLayer> > tiles(m_tiles.size());

Vector<RetainPtr<WebTileLayer> > could use a typedef.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:323
> +unsigned TileCache::blankPixelCountForTiles(Vector<RetainPtr<WebTileLayer> > tiles, IntRect visibleRect, IntPoint tileTranslation)

Please pass the vector by const reference.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:327
> +    for (Vector<RetainPtr<WebTileLayer> >::const_iterator it = tiles.begin(), end = tiles.end(); it != end; ++it) {

Does declaring end here work on all versions of gcc/clang we care about? We don't normally do it this way.

[Tim Horton]

(In reply to comment #13)
> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:327
> > +    for (Vector<RetainPtr<WebTileLayer> >::const_iterator it = tiles.begin(), end = tiles.end(); it != end; ++it) {
> 
> Does declaring end here work on all versions of gcc/clang we care about? We don't normally do it this way.

Anders uses that in a few places in TileCache.mm; I'm pretty sure it works everywhere. I don't think it's an unusual feature. Perhaps we should use it more often :)

Thanks, Simon!

[Tim Horton]

I'm going to let the Mac EWS finish before landing since it already found one build failure I don't see with my clang.

[Tim Horton]

(In reply to comment #14)
> (In reply to comment #13)
> > > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:327
> > > +    for (Vector<RetainPtr<WebTileLayer> >::const_iterator it = tiles.begin(), end = tiles.end(); it != end; ++it) {
> > 
> > Does declaring end here work on all versions of gcc/clang we care about? We don't normally do it this way.
> 
> Anders uses that in a few places in TileCache.mm; I'm pretty sure it works everywhere. I don't think it's an unusual feature. Perhaps we should use it more often :)
> 
> Thanks, Simon!

smfr: r still=you with s/currentTime/monotonicallyIncreasingTime/? :) (still waiting for ews and finding bugs where there are none)

[Tim Horton]

http://trac.webkit.org/changeset/125156

[Simon Fraser (smfr)]

Sure!