Bug 47320 – [Performance] Only call sendContentResizeNotification when the scrollbar actually did change

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ryuan Choi]

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.

[Ryuan Choi]

Created attachment 70030 [details]
Patch

[Antonio Gomes]

Comment on attachment 70030 [details]
Patch

Seems reasonable to me. Did you see any perf gain?

[Ryuan Choi]

(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.

[Kenneth Rohde Christiansen]

(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.

[Ryuan Choi]

(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.

[Kenneth Rohde Christiansen]

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?

[Antonio Gomes]

Generally, a user-feel improvement is valid, but there are many more accurate way to measure a performance improvement, e.g. frames per second ...

[Ryuan Choi]

(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%

[Eric Seidel (no email)]

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.

[Antonio Gomes]

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.

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 76465 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Antonio Gomes]

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()

[Antonio Gomes]

> 
> ASSERT(avoidScrollbarCreation())

Of course I meant ASSERT(!avoidScrollbarCreation())

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 78365 [details]
Patch

[Brent Fulgham]

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

[Ryuan Choi]

Created attachment 89577 [details]
Patch

[Ryuan Choi]

(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.

[Eric Seidel (no email)]

James should see this go by.

[Eric Seidel (no email)]

Comment on attachment 89577 [details]
Patch

Seems reasonable to me.  But I know very little about the scrollbar code.

[Eric Seidel (no email)]

James, Hyatt, Peter?  Someone who's worked in scrollbars?

[Peter Kasting]

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.

[Ryuan Choi]

Created attachment 99804 [details]
Patch

[Ryuan Choi]

(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.

[Peter Kasting]

Comment on attachment 99804 [details]
Patch

Informal r+ from me; you'll need a real reviewer to do the formal r+.

[Eric Seidel (no email)]

Comment on attachment 99804 [details]
Patch

Thanks Peter and Ryuan.

[WebKit Review Bot]

Comment on attachment 99804 [details]
Patch

Clearing flags on attachment: 99804

Committed r90478: <http://trac.webkit.org/changeset/90478>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.