Bug 70407 – [TexMap][QT] PageClientQt should skip redundant sync requests during Accelerated Composition.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Dongseong Hwang]

The 'shouldSync' variable already exist, but does not be used properly in PageClientQt.cpp.

[Dongseong Hwang]

Created attachment 111585 [details]
patch

[Noam Rosenthal]

Comment on attachment 111585 [details]
patch

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

I don't understand the problem, and the patch is either way incorrect.
syncLayers should turn the shoulsSync flag off, as by that point we have already synced. There might be an issue with animations, in that case setting the flag should only occur inside the if loop in line 254.

> Source/WebKit/qt/ChangeLog:8
> +        PageClientQt should receives the sync request after actual TexMap drawing.

This sentence does not make sense in English.

[Dongseong Hwang]

The "shouldSync" is not used in current code. I wonder which "shouldSync" affects in code.

I thought you created "shouldSync" for following purpose.
1. WebCore callbacks PageClientQGraphicsWidget::syncLayers() in order to invalidate.
2. Before invalidating WebCore callbacks PageClientQGraphicsWidget::syncLayers() so many times.
3. PageClientQGraphicsWidget should invalidate only one time for all sync requests.

I experienced performance largely increased by this patch in my project based on private SDL port in the following site. In detail, from 3fps to 20fps.
http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html

This site requests more than 600 sync requests before invalidating actually.

I'm sorry for my poor English. English is more difficult than C++ to Asian.

[Noam Rosenthal]

> I thought you created "shouldSync" for following purpose.
> 1. WebCore callbacks PageClientQGraphicsWidget::syncLayers() in order to invalidate.
No, it should callback markForSync, which starts a timer that calls syncLayers.
The bug is valid but the solution is incorrect. The problem is that we call syncLayers instead of markForSync.


> I'm sorry for my poor English. English is more difficult than C++ to Asian.
No worries; I'm just trying to make sure people can understand the Changelog. Sorry for sounding judgmental!

[Dongseong Hwang]

You're welcome.

I understood "shouldSync" is for makeForSync, but I did not understand yet why there is not something like this code.
if (shouldSync)
    doSomething();

Anyway, I am not sure whether the situation related to 600 sync as I mentioned is bug or not.
If it is bug, is it proper to add PageClientQGraphicsWidget's member variable like shouldSync in order to reduce redundant markForSync calls?

[Noam Rosenthal]

(In reply to comment #5)
> You're welcome.
> 
> I understood "shouldSync" is for makeForSync, but I did not understand yet why there is not something like this code.
> if (shouldSync)
>     doSomething();
> 
> Anyway, I am not sure whether the situation related to 600 sync as I mentioned is bug or not.
> If it is bug, is it proper to add PageClientQGraphicsWidget's member variable like shouldSync in order to reduce redundant markForSync calls?

Hmmm... looking at it again some of that logic doesn't make sense, and might be leftovers from a previous iteration.
But if we fix it let's do it right!
Maybe the right fix should be to eliminate shouldSync, and to simply start the timer on markForSync?

[Dongseong Hwang]

(In reply to comment #6)
> (In reply to comment #5)
> > You're welcome.
> > 
> > I understood "shouldSync" is for makeForSync, but I did not understand yet why there is not something like this code.
> > if (shouldSync)
> >     doSomething();
> > 
> > Anyway, I am not sure whether the situation related to 600 sync as I mentioned is bug or not.
> > If it is bug, is it proper to add PageClientQGraphicsWidget's member variable like shouldSync in order to reduce redundant markForSync calls?
> 
> Hmmm... looking at it again some of that logic doesn't make sense, and might be leftovers from a previous iteration.
> But if we fix it let's do it right!
> Maybe the right fix should be to eliminate shouldSync, and to simply start the timer on markForSync?

line 302, syncTimer.startOneShot(0);
markForSync already simply starts the timer. What is mean?

I think I did not understand wholly what you said, so I hesitated to response more because it might be small problem or not problem.
You means it is better to resolve this bug because of INVALID?

[Noam Rosenthal]

> I think I did not understand wholly what you said, so I hesitated to response more because it might be small problem or not problem.
> You means it is better to resolve this bug because of INVALID?
No. It means that the fix is a lot simpler - return early in markForSync if the syncTimer is already active.
Something like

if (syncTimer.isActive())
    return;

You also might want to stop the timer in ::syncLayers.

The bug is actually that we rely on Timer::startOneShot to do nothing if the timer is already running, but that's not how Timer::startOneShot works.

[Dongseong Hwang]

Created attachment 115304 [details]
patch

This patch increased performance in following site that is similar to Falling Leaves.
http://www.dorothybrowser.com/test/joybro/leaves/
When clicking '400 leaves', you can feel less lag.

[Noam Rosenthal]

Comment on attachment 115304 [details]
patch

Good patch! Please add a Changelog and I'd be happy to review :)

[Dongseong Hwang]

Created attachment 115310 [details]
patch

I made mistake. :)

[Dongseong Hwang]

Created attachment 115315 [details]
patch

I think this patch should apply PageClientQWidget, also.

[Noam Rosenthal]

Comment on attachment 115315 [details]
patch

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

Almost there. I allowed myself to help reword the changelog entry, trying to make it more concise and clear.

> Source/WebKit/qt/ChangeLog:12
> +        It relieves the load of calling PageClientQWidget::syncLayers redundantly.
> +        It increases TexMap's performance when PageClientQGraphicsWidget::markForSync is
> +        called many times, especially the time to prepare layers and animations.
> +        The bug is actually that we rely on Timer::startOneShot to do nothing if the
> +        timer is already running, but the Timer fires same times that
> +        Timer::startOneShot was called.

How about "Make sure we only activate the synchronization timer in PageClientQWidget/PageClientQGraphicsWidget if it's not already active, otherwise syncLayers may be called redundantly."

[Dongseong Hwang]

Created attachment 115320 [details]
patch

Thank you for Good Changelog.

[WebKit Review Bot]

Comment on attachment 115320 [details]
patch

Clearing flags on attachment: 115320

Committed r100410: <http://trac.webkit.org/changeset/100410>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.