RESOLVED FIXED 99768
We should limit the tile cache coverage when a page can't take advantage of fast tile scrolling anyway
https://bugs.webkit.org/show_bug.cgi?id=99768
Summary We should limit the tile cache coverage when a page can't take advantage of f...
Beth Dakin
Reported 2012-10-18 15:47:26 PDT
Right now, we only limit the size of the tile cache if a page cannot scroll at all. We should limit it whenever a page can't take advantage of fast tile scrolling. Making this change seems to be a performance improvement on pages that are very painting-intensive such as Facebook.
Attachments
Patch (5.40 KB, patch)
2012-10-18 16:02 PDT, Beth Dakin
no flags
Patch (7.68 KB, patch)
2012-10-18 17:58 PDT, Beth Dakin
simon.fraser: review+
Beth Dakin
Comment 1 2012-10-18 16:02:17 PDT
Simon Fraser (smfr)
Comment 2 2012-10-18 16:08:32 PDT
Comment on attachment 169497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction. > Source/WebCore/page/FrameView.cpp:797 > + if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) { > + if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers()) > + return true; > + } Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling?
Beth Dakin
Comment 3 2012-10-18 16:10:56 PDT
(In reply to comment #2) > (From update of attachment 169497 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review > > I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction. > > > Source/WebCore/page/FrameView.cpp:797 > > + if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) { > > + if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers()) > > + return true; > > + } > > Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling? It is. That was my plan originally, actually, but I talked with Anders in person, and I got the impression that he liked the idea of keeping this all in FrameView. Maybe I misinterpreted him though. I will give him a chance to defend the idea or claim I am crazy since I only did it this way because I thought he liked it better :-)
Anders Carlsson
Comment 4 2012-10-18 17:17:44 PDT
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 169497 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review > > > > I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction. > > > > > Source/WebCore/page/FrameView.cpp:797 > > > + if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) { > > > + if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers()) > > > + return true; > > > + } > > > > Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling? > > It is. That was my plan originally, actually, but I talked with Anders in person, and I got the impression that he liked the idea of keeping this all in FrameView. Maybe I misinterpreted him though. I will give him a chance to defend the idea or claim I am crazy since I only did it this way because I thought he liked it better :-) I'll do you one better and claim that _I_ am crazy. It's clearly better to keep the logic in ScrollingCoordinator.
Tim Horton
Comment 5 2012-10-18 17:19:20 PDT
(In reply to comment #2) > (From update of attachment 169497 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review > > I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction. > > > Source/WebCore/page/FrameView.cpp:797 > > + if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) { > > + if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers()) > > + return true; > > + } > > Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling? Does seem odd, especially since there are more reasons that shouldUpdateScrollLayerPositionOnMainThread could be true which aren't covered here...
Beth Dakin
Comment 6 2012-10-18 17:58:25 PDT
Simon Fraser (smfr)
Comment 7 2012-10-19 13:06:48 PDT
Comment on attachment 169521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169521&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:282 > +MainThreadScrollingReasons ScrollingCoordinator::shouldUpdateScrollLayerPositionOnMainThread() const Returning the MainThreadScrollingReasons from a 'should' method is a bit confusing; I think this would be clearer as two methods (possibly inline): MainThreadScrollingReasons mainThreadScrollingReasons() const; bool shouldUpdateScrollLayerPositionOnMainThread() const { return mainThreadScrollingReasons() != 0; }
Beth Dakin
Comment 8 2012-10-19 13:33:50 PDT
Note You need to log in before you can comment on or make changes to this bug.