WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(7.68 KB, patch)
2012-10-18 17:58 PDT
,
Beth Dakin
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2012-10-18 16:02:17 PDT
Created
attachment 169497
[details]
Patch
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
Created
attachment 169521
[details]
Patch
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
Thanks, Simon!
http://trac.webkit.org/changeset/131939
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug