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).
This can't use a log channel because it needs to work in release mode.
Created attachment 157297 [details] patch
Created attachment 157298 [details] style
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
Created attachment 157321 [details] revised patch
(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.
Comment on attachment 157321 [details] revised patch Attachment 157321 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13462323
Created attachment 157362 [details] humorously, fix mac build
Created attachment 157368 [details] negate the offset like it used to be
Created attachment 157373 [details] typo'd 'layer' for 'sublayer'
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.
(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).
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.
(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!
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.
(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)
http://trac.webkit.org/changeset/125156
Sure!