The 'shouldSync' variable already exist, but does not be used properly in PageClientQt.cpp.
Created attachment 111585 [details] patch
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.
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.
> 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!
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?
(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?
(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?
> 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.
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.
Comment on attachment 115304 [details] patch Good patch! Please add a Changelog and I'd be happy to review :)
Created attachment 115310 [details] patch I made mistake. :)
Created attachment 115315 [details] patch I think this patch should apply PageClientQWidget, also.
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."
Created attachment 115320 [details] patch Thank you for Good Changelog.
Comment on attachment 115320 [details] patch Clearing flags on attachment: 115320 Committed r100410: <http://trac.webkit.org/changeset/100410>
All reviewed patches have been landed. Closing bug.