Bug 47320 - [Performance] Only call sendContentResizeNotification when the scrollbar actually did change
Summary: [Performance] Only call sendContentResizeNotification when the scrollbar actu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2010-10-06 19:20 PDT by Ryuan Choi
Modified: 2011-07-06 12:05 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.01 KB, patch)
2010-10-06 19:50 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2010-12-13 16:37 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (4.24 KB, patch)
2011-01-09 19:08 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (2.48 KB, patch)
2011-04-14 06:47 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2011-07-06 02:56 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2010-10-06 19:20:07 PDT
In ScrollView::updateScrollbars, it called visibleContentsResized when scrollbars are shown or hidden.

but, frameFlattening option can block creation of scrollbar.
so we don't need to call visibleContentsResized.
Comment 1 Ryuan Choi 2010-10-06 19:50:19 PDT
Created attachment 70030 [details]
Patch
Comment 2 Antonio Gomes 2010-10-07 21:10:45 PDT
Comment on attachment 70030 [details]
Patch

Seems reasonable to me. Did you see any perf gain?
Comment 3 Ryuan Choi 2010-10-10 01:38:18 PDT
(In reply to comment #2)
> (From update of attachment 70030 [details])
> Seems reasonable to me. Did you see any perf gain?

Sorry about late answer.

yes, I saw sluggish scrolling at our mobile browser using frameflattening option.
Those sites contains frameset or iframe.
And this patch scrolling this kinds of sites make very faster than before.
Comment 4 Kenneth Rohde Christiansen 2010-10-10 05:35:35 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 70030 [details] [details])
> > Seems reasonable to me. Did you see any perf gain?
> 
> Sorry about late answer.
> 
> yes, I saw sluggish scrolling at our mobile browser using frameflattening option.
> Those sites contains frameset or iframe.
> And this patch scrolling this kinds of sites make very faster than before.

Strange, only relayout could affect scrolling, so are these sites for some reason relayouting the whole time? Maybe it is better to look into this? or optimize the frame flattening layout code.

Also, frame flattening is needed for a nice touch interaction model. That is way more important than the performance.
Comment 5 Ryuan Choi 2010-10-10 19:12:10 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 70030 [details] [details] [details])
> > > Seems reasonable to me. Did you see any perf gain?
> > 
> > Sorry about late answer.
> > 
> > yes, I saw sluggish scrolling at our mobile browser using frameflattening option.
> > Those sites contains frameset or iframe.
> > And this patch scrolling this kinds of sites make very faster than before.
> 
> Strange, only relayout could affect scrolling, so are these sites for some reason relayouting the whole time? Maybe it is better to look into this? or optimize the frame flattening layout code.
> 
you're right. this patch want not to relayout if scrollbar wasn't changed.
If not, ScrollView calls contentsResized and visiblieContentsResized although scrollbar wasn't created because of frameflattening.

And my test site is http://postech.edu/

> Also, frame flattening is needed for a nice touch interaction model. That is way more important than the performance.

Yes, so we want to use frame flattening. But I found that it's too sluggish case and I believe that this patch remove it without any side effect.
Comment 6 Kenneth Rohde Christiansen 2010-10-10 19:47:31 PDT
Ah, I understood now, so with this patch scrolling performance with frame flattening is improved.

Hyatt, can you have a look at this one, please?
Comment 7 Antonio Gomes 2010-10-10 21:57:02 PDT
Generally, a user-feel improvement is valid, but there are many more accurate way to measure a performance improvement, e.g. frames per second ...
Comment 8 Ryuan Choi 2010-10-11 06:44:10 PDT
(In reply to comment #7)
> Generally, a user-feel improvement is valid, but there are many more accurate way to measure a performance improvement, e.g. frames per second ...

Hi, I tested using our mobile browser implemented by EFL.
I don't have idea which browser(not EFL) can test this.
I use ECORE_EVAS_FPS_DEBUG to get below data without any changes.

without our patch.
FRAME: 454, FPS: 28.0, RTIME  65%
FRAME: 469, FPS: 28.7, RTIME  64%
FRAME: 484, FPS: 28.3, RTIME  65%
FRAME: 499, FPS: 28.9, RTIME  65%
FRAME: 514, FPS: 28.4, RTIME  64%
FRAME: 529, FPS: 28.4, RTIME  64%
FRAME: 544, FPS: 28.4, RTIME  66%
FRAME: 559, FPS: 28.1, RTIME  66%
FRAME: 574, FPS: 29.3, RTIME  65%
FRAME: 589, FPS: 28.5, RTIME  67%
FRAME: 604, FPS: 28.4, RTIME  65%
FRAME: 619, FPS: 28.6, RTIME  65%
FRAME: 634, FPS: 28.8, RTIME  64%
FRAME: 649, FPS: 29.0, RTIME  68%
FRAME: 664, FPS: 28.7, RTIME  65%
FRAME: 679, FPS: 28.6, RTIME  64%
FRAME: 694, FPS: 28.6, RTIME  66%
FRAME: 708, FPS: 27.8, RTIME  64%
FRAME: 723, FPS: 29.0, RTIME  68%
FRAME: 738, FPS: 28.4, RTIME  66%
FRAME: 753, FPS: 28.7, RTIME  65%
FRAME: 768, FPS: 28.6, RTIME  66%
FRAME: 784, FPS: 31.3, RTIME  69%
FRAME: 800, FPS: 31.4, RTIME  69%
FRAME: 817, FPS: 33.4, RTIME  63%

with our patch
FRAME: 374, FPS: 23.8, RTIME  33%
FRAME: 404, FPS: 59.4, RTIME  77%
FRAME: 435, FPS: 62.0, RTIME  77%
FRAME: 465, FPS: 58.8, RTIME  76%
FRAME: 495, FPS: 59.4, RTIME  76%
FRAME: 526, FPS: 60.3, RTIME  75%
FRAME: 556, FPS: 59.9, RTIME  75%
FRAME: 588, FPS: 63.1, RTIME  79%
FRAME: 619, FPS: 62.0, RTIME  77%
FRAME: 651, FPS: 62.9, RTIME  78%
FRAME: 682, FPS: 61.5, RTIME  77%
FRAME: 714, FPS: 61.9, RTIME  76%
FRAME: 745, FPS: 61.8, RTIME  78%
FRAME: 777, FPS: 63.2, RTIME  77%
FRAME: 809, FPS: 63.1, RTIME  77%
FRAME: 840, FPS: 61.0, RTIME  79%
FRAME: 877, FPS: 73.8, RTIME  83%
Comment 9 Eric Seidel (no email) 2010-12-13 00:40:36 PST
Comment on attachment 70030 [details]
Patch

Should the caller just check avoidScrollbarCreation?  Or maybe sendContentResizedNotification should just be set to the return value of the setHasHorizontalScrollbar call.
Comment 10 Antonio Gomes 2010-12-13 07:27:43 PST
Comment on attachment 70030 [details]
Patch

Ryan, I know your intentions and it generally makes sense. But I think doing as Eric suggested it clearer.

It is strange that a setter returns something.
Comment 11 Ryuan Choi 2010-12-13 15:55:22 PST
(In reply to comment #9)
> (From update of attachment 70030 [details])
> Should the caller just check avoidScrollbarCreation?  Or maybe sendContentResizedNotification should just be set to the return value of the setHasHorizontalScrollbar call.

Thank you for your comment.
I realized my mistake.

(In reply to comment #10)
> (From update of attachment 70030 [details])
> Ryan, I know your intentions and it generally makes sense. But I think doing as Eric suggested it clearer.
> 
> It is strange that a setter returns something.

I'll make newer like Eric mentioned.
Thanks.
Comment 12 Ryuan Choi 2010-12-13 16:37:05 PST
Created attachment 76465 [details]
Patch
Comment 13 Kenneth Rohde Christiansen 2010-12-13 16:55:13 PST
Comment on attachment 76465 [details]
Patch

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

> WebCore/ChangeLog:15
> +

You have to verify that you do not break the frame flattening tests with this change. Are you running the layout tests?
Comment 14 Antonio Gomes 2010-12-13 16:59:38 PST
Comment on attachment 76465 [details]
Patch

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

Also as kenneth said, Qt Gtk and Mac DRT test the flame flattening feature. Please, make sure there is no regression.

> WebCore/platform/ScrollView.cpp:102
>  void ScrollView::setHasVerticalScrollbar(bool hasBar)
>  {
> -    if (hasBar && avoidScrollbarCreation())
> +    if (hasBar)

Well, if we go for it, I would also suggest to add an assert here:

ASSERT(avoidScrollbarCreation())

That way we ensure that no one is calling setHasVerticalScrollbar() without previously calling avoidScrollbarCreation()
Comment 15 Antonio Gomes 2010-12-13 17:00:13 PST
> 
> ASSERT(avoidScrollbarCreation())

Of course I meant ASSERT(!avoidScrollbarCreation())
Comment 16 Ryuan Choi 2010-12-13 20:08:15 PST
(In reply to comment #13)
> (From update of attachment 76465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76465&action=review
> 
> > WebCore/ChangeLog:15
> > +
> 
> You have to verify that you do not break the frame flattening tests with this change. Are you running the layout tests?

Sorry.
With layout tests, I got failed. I think that I should investigate more.

(In reply to comment #14)
> (From update of attachment 76465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76465&action=review
> 
> Also as kenneth said, Qt Gtk and Mac DRT test the flame flattening feature. Please, make sure there is no regression.
> 
> > WebCore/platform/ScrollView.cpp:102
> >  void ScrollView::setHasVerticalScrollbar(bool hasBar)
> >  {
> > -    if (hasBar && avoidScrollbarCreation())
> > +    if (hasBar)
> 
> Well, if we go for it, I would also suggest to add an assert here:
> 
> ASSERT(avoidScrollbarCreation())
> 
> That way we ensure that no one is calling setHasVerticalScrollbar() without previously calling avoidScrollbarCreation()

Sorry, This patch was wrong.
I'll prepare new patch after those testing.
Comment 17 Ryuan Choi 2011-01-09 19:08:54 PST
Created attachment 78365 [details]
Patch
Comment 18 Brent Fulgham 2011-04-11 13:41:20 PDT
Comment on attachment 78365 [details]
Patch

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

> Source/WebCore/platform/ScrollView.cpp:89
> +    ASSERT(!(hasBar && avoidScrollbarCreation()));

I don't understand why the "hasBar && avoidScrollbarCreation()" test no longer returns.  You assert if this case is true -- do you want to execute the code under this condition?

> Source/WebCore/platform/ScrollView.cpp:105
> +    ASSERT(!(hasBar && avoidScrollbarCreation()));

Ditto
Comment 19 Ryuan Choi 2011-04-14 06:47:04 PDT
Created attachment 89577 [details]
Patch
Comment 20 Ryuan Choi 2011-04-14 06:54:10 PDT
(In reply to comment #18)
> (From update of attachment 78365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78365&action=review
> 
> > Source/WebCore/platform/ScrollView.cpp:89
> > +    ASSERT(!(hasBar && avoidScrollbarCreation()));
> 
> I don't understand why the "hasBar && avoidScrollbarCreation()" test no longer returns.  You assert if this case is true -- do you want to execute the code under this condition?
> 
> > Source/WebCore/platform/ScrollView.cpp:105
> > +    ASSERT(!(hasBar && avoidScrollbarCreation()));
> 
> Ditto

Sorry about late answer.

This patch is old and I can't assure any side effects.
So I simplified my patch to explain what I really want.
It will be fine, but check hasBar && avoidScrollbarCreation two times.
Comment 21 Eric Seidel (no email) 2011-05-23 15:17:06 PDT
James should see this go by.
Comment 22 Eric Seidel (no email) 2011-07-05 20:42:13 PDT
Comment on attachment 89577 [details]
Patch

Seems reasonable to me.  But I know very little about the scrollbar code.
Comment 23 Eric Seidel (no email) 2011-07-05 20:42:44 PDT
James, Hyatt, Peter?  Someone who's worked in scrollbars?
Comment 24 Peter Kasting 2011-07-05 23:15:32 PDT
Comment on attachment 89577 [details]
Patch

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

In theory this seems OK, but I'm not a WebKit reviewer.

> Source/WebCore/platform/ScrollView.cpp:500
>          if (hasHorizontalScrollbar != newHasHorizontalScrollbar) {

Nit: I think it might be clearer to write this block as follows:

if (hasHorizontalScrollbar != newHasHorizontalScrollbar && (hasHorizontalScrollbar || !avoidScrollbarCreation())) {
    ... original block contents here...
}

...and the same for the case below.  This makes it a bit clearer that we don't want to do anything in the case where we're supposed to avoid scrollbar creation.

I would also like to see setHasXXXScrollbar() converted from conditional-checking (hasBar && avoidScrollbarCreation()) into asserting that that combination is false.  Either this is safe because these are the only places which call these functions, or else your patch should probably be expanded to the other callers thereof.  In either case, making this expectation part of those functions' contracts seems clearer and safer long-term to me.
Comment 25 Ryuan Choi 2011-07-06 02:56:05 PDT
Created attachment 99804 [details]
Patch
Comment 26 Ryuan Choi 2011-07-06 02:57:30 PDT
(In reply to comment #24)
> (From update of attachment 89577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89577&action=review
> 
> In theory this seems OK, but I'm not a WebKit reviewer.
> 
> > Source/WebCore/platform/ScrollView.cpp:500
> >          if (hasHorizontalScrollbar != newHasHorizontalScrollbar) {
> 
> Nit: I think it might be clearer to write this block as follows:
> 
> if (hasHorizontalScrollbar != newHasHorizontalScrollbar && (hasHorizontalScrollbar || !avoidScrollbarCreation())) {
>     ... original block contents here...
> }
> 
> ...and the same for the case below.  This makes it a bit clearer that we don't want to do anything in the case where we're supposed to avoid scrollbar creation.
> 
> I would also like to see setHasXXXScrollbar() converted from conditional-checking (hasBar && avoidScrollbarCreation()) into asserting that that combination is false.  Either this is safe because these are the only places which call these functions, or else your patch should probably be expanded to the other callers thereof.  In either case, making this expectation part of those functions' contracts seems clearer and safer long-term to me.

Thank you for your comment.
I updated like you mentioned.
Comment 27 Peter Kasting 2011-07-06 10:58:03 PDT
Comment on attachment 99804 [details]
Patch

Informal r+ from me; you'll need a real reviewer to do the formal r+.
Comment 28 Eric Seidel (no email) 2011-07-06 11:05:01 PDT
Comment on attachment 99804 [details]
Patch

Thanks Peter and Ryuan.
Comment 29 WebKit Review Bot 2011-07-06 12:05:01 PDT
Comment on attachment 99804 [details]
Patch

Clearing flags on attachment: 99804

Committed r90478: <http://trac.webkit.org/changeset/90478>
Comment 30 WebKit Review Bot 2011-07-06 12:05:11 PDT
All reviewed patches have been landed.  Closing bug.