Bug 34062 - [Qt] Tuning and optimizations to GraphicsLayerQt
Summary: [Qt] Tuning and optimizations to GraphicsLayerQt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-24 18:18 PST by Noam Rosenthal
Modified: 2010-02-04 08:56 PST (History)
5 users (show)

See Also:


Attachments
First round of performance tunings for QtWebkit accelerated compositing (25.73 KB, patch)
2010-01-24 19:54 PST, Noam Rosenthal
ariya.hidayat: review-
Details | Formatted Diff | Diff
tuning / optimiization to GraphicsLayerQt + added a comment about actual FPS improvement to the ChangeLog (25.81 KB, patch)
2010-02-01 11:44 PST, Noam Rosenthal
ariya.hidayat: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Performance tuning to GraphicsLayerQt (26.45 KB, patch)
2010-02-02 12:19 PST, Noam Rosenthal
kenneth: review-
Details | Formatted Diff | Diff
Performance tuning to GraphicsLayerQt: Some style fixes + correct changelog (24.09 KB, patch)
2010-02-02 14:05 PST, Noam Rosenthal
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fixed a glitch in the actual diff file.... strange (24.07 KB, patch)
2010-02-02 20:39 PST, Noam Rosenthal
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
re-applying, uploaded wrong file (23.74 KB, patch)
2010-02-03 07:18 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Fixed a build issue (24.27 KB, patch)
2010-02-03 13:23 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-01-24 18:18:55 PST
Tune the caching methodologies and flow of GraphicsLayerQt for better paint performance
Comment 1 Noam Rosenthal 2010-01-24 19:54:41 PST
Created attachment 47307 [details]
First round of performance tunings for QtWebkit accelerated compositing

Fixed a few issues like over-use of QTimer::singleShot, re-caching when it wasn't needed, and edge cases with transform animations that weren't supported.
Comment 2 Ariya Hidayat 2010-01-27 10:55:12 PST
Comment on attachment 47307 [details]
First round of performance tunings for QtWebkit accelerated compositing

I like the optimization :) Good job, No'am.

My minor concern: can we have a benchmark for this? I mean, there is a few manual tests out there (I'm sure you use some of them) but I am referring to QBENCHMARK-type of tests.

A much more quantified speed-up mentioned in the ChangeLog will be very appreciated, IMO.

r- for this, otherwise looks good!
Comment 3 Noam Rosenthal 2010-01-28 07:22:01 PST
Ariya: To my understanding, QBENCHMARK is more about low-level stuff like measuring how much time it takes to render text 10,000 times, it's hard for me to envision how I would use it for something high level like CSS animations; maybe someone else has an idea?

The real benchmark for this is FPS and not msecs-per-low-level-operation.
What I can add to this is my css animation test suite (which currently lives somewhere on gitorious) into WebKit/qt/tests with some C++ code that measures the FPS with different QGV configurations.

However, for this specific patch, would a more general statement to the changelog be sufficient, keeping the benchmark for the next commit, or should I bundle them into one commit?
Comment 4 Ariya Hidayat 2010-02-01 11:27:30 PST
Comment on attachment 47307 [details]
First round of performance tunings for QtWebkit accelerated compositing

Hmm, in this case, mentioning the speed difference (before and after FPS) would be sufficient in the ChangeLog.

Moving your benchmark tests to WebKit/qt/tests can be in a separate commit (and no need to rush for that).
Comment 5 Noam Rosenthal 2010-02-01 11:44:53 PST
Created attachment 47858 [details]
tuning / optimiization to GraphicsLayerQt + added a comment about actual FPS improvement to the ChangeLog

I've added a comment to the changelog mentioning that this change improves performance of a rotating table from 40FPS to 50FPS on Maemo5. Would that do?
My next commit would include a better FPS benchmark for CSS animations.
Comment 6 Ariya Hidayat 2010-02-01 16:07:14 PST
Comment on attachment 47858 [details]
tuning / optimiization to GraphicsLayerQt + added a comment about actual FPS improvement to the ChangeLog

> +        [Qt] Tuning and optimizations to GraphicsLayerQt. Reduce unnecessary
> +        recaching, remove QTimer::singleShot and QPixmap::scaled, more
> +        accurate strategy of handling transform operation blends. Rotating a bordered-table, for example, now runs at 50FPS instead of 40FPS on Maemo5.

You may want to wrap the last line properly.

Otherwise, LGTM.
Comment 7 Noam Rosenthal 2010-02-02 09:06:25 PST
How does this get into the commit queue? Do I need to fix that line wraps and ask for a re-review? or do I just need to wait? :)
Comment 8 Noam Rosenthal 2010-02-02 09:11:01 PST
Thanks Kenneth!
Comment 9 WebKit Commit Bot 2010-02-02 09:43:35 PST
Comment on attachment 47858 [details]
tuning / optimiization to GraphicsLayerQt + added a comment about actual FPS improvement to the ChangeLog

Rejecting patch 47858 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
re/ChangeLog
	M	WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
	M	WebKit/qt/Api/qgraphicswebview.cpp
	M	WebKit/qt/ChangeLog
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 558


Full output: http://webkit-commit-queue.appspot.com/results/230652
Comment 10 Noam Rosenthal 2010-02-02 09:51:01 PST
Interesting... someone needs to change the Reviewed-by thingy in the changelogs?
Comment 11 Laszlo Gombos 2010-02-02 11:06:15 PST
Noam, I think the failure comes from " No new tests. (OOPS!)" in the ChangeLog. That is something you should be able to fix.
Comment 12 Noam Rosenthal 2010-02-02 12:19:28 PST
Created attachment 47963 [details]
Performance tuning to GraphicsLayerQt

Fixed the line-wraps and put better description of testing in the commit log. No new code.
Comment 13 Kenneth Rohde Christiansen 2010-02-02 12:39:30 PST
Comment on attachment 47963 [details]
Performance tuning to GraphicsLayerQt

Some minor comments

>  
>  class OpacityAnimationQt : public AnimationQt<qreal> {
>  public:
>      OpacityAnimationQt(GraphicsLayerQtImpl* layer, const KeyframeValueList& values, const IntSize& boxSize, const Animation* anim, const QString & name)
> -         : AnimationQt<qreal>(layer, values, boxSize, anim, name)
> +                : AnimationQt<qreal>(layer, values, boxSize, anim, name)

Wrong indentation


> -    } else {
> +    } else
>          for (QList<QWeakPointer<QAbstractAnimation> >::iterator it = m_impl->m_animations.begin(); it != m_impl->m_animations.end(); ++it) {

The for spends multiply lines, so the else would need {! Please look at the style guide, other examples include

correct:
if (true)
    doSomething

wrong:
if (true)
   // do something
   doSomething

correct:

if (true) {
   // do something
   doSomething
}


> @@ -129,6 +131,9 @@ public:
>      // compositor telling us to do so. We'll get that call from ChromeClientQt

// compositor notifies us throught ChromeClientQt when we need to synchronize 
??? 

>      bool shouldSync;
Comment 14 Eric Seidel (no email) 2010-02-02 13:55:51 PST
(In reply to comment #11)
> Noam, I think the failure comes from " No new tests. (OOPS!)" in the ChangeLog.
> That is something you should be able to fix.

That's correct.  The commit-queue knows how to fix the Reviewed by line, but won't try to fix the lack of test explanation. :)

See http://webkit.org/coding/contributing.html#changelogs
Comment 15 Noam Rosenthal 2010-02-02 14:05:13 PST
Created attachment 47973 [details]
Performance tuning to GraphicsLayerQt: Some style fixes + correct changelog

Absolutely right - my bad.
Hopefully this patch should do it.
Comment 16 WebKit Commit Bot 2010-02-02 20:15:03 PST
Comment on attachment 47973 [details]
Performance tuning to GraphicsLayerQt: Some style fixes + correct changelog

Rejecting patch 47973 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 1
Last 500 characters of output:
z 3.
patching file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
Hunk #19 FAILED at 1022.
1 out of 21 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp.rej
patching file WebKit/qt/Api/qgraphicswebview.cpp
Hunk #3 succeeded at 134 with fuzz 1 (offset 3 lines).
Hunk #4 succeeded at 183 (offset 2 lines).
Hunk #5 succeeded at 229 (offset 2 lines).
Hunk #6 succeeded at 448 (offset 7 lines).
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Full output: http://webkit-commit-queue.appspot.com/results/232050
Comment 17 Noam Rosenthal 2010-02-02 20:39:18 PST
Created attachment 47991 [details]
Fixed a glitch in the actual diff file.... strange

Same code - just fixed a glitch in the diff file. 
It definitely works for me, hope it works for mr. review-bot :)
Comment 18 Laszlo Gombos 2010-02-02 21:16:11 PST
Noam, I think you would need to merge with WebKit trunk, resolve the conflicts in GraphicsLayerQt.cpp and upload the patch again.
Comment 19 Noam Rosenthal 2010-02-02 21:29:33 PST
yes, that's what I just did with the new patch :)
Comment 20 WebKit Commit Bot 2010-02-03 05:29:18 PST
Comment on attachment 47991 [details]
Fixed a glitch in the actual diff file.... strange

Rejecting patch 47991 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 1
Last 500 characters of output:
z 3.
patching file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
Hunk #19 FAILED at 1022.
1 out of 21 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp.rej
patching file WebKit/qt/Api/qgraphicswebview.cpp
Hunk #3 succeeded at 134 with fuzz 1 (offset 3 lines).
Hunk #4 succeeded at 183 (offset 2 lines).
Hunk #5 succeeded at 229 (offset 2 lines).
Hunk #6 succeeded at 448 (offset 7 lines).
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Full output: http://webkit-commit-queue.appspot.com/results/232358
Comment 21 Noam Rosenthal 2010-02-03 07:18:12 PST
Created attachment 48024 [details]
re-applying, uploaded wrong file

trying again - these are just patching mechanics problem... sorry for whoever this is taking time from :)
Comment 22 Laszlo Gombos 2010-02-03 07:44:14 PST
Comment on attachment 48024 [details]
re-applying, uploaded wrong file

This time it looks like the patch applies cleanly.
Comment 23 WebKit Commit Bot 2010-02-03 08:01:18 PST
Comment on attachment 48024 [details]
re-applying, uploaded wrong file

Clearing flags on attachment: 48024

Committed r54281: <http://trac.webkit.org/changeset/54281>
Comment 24 WebKit Commit Bot 2010-02-03 08:01:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Andras Becsi 2010-02-03 10:11:38 PST
Unfortunately we had to roll-out the patch in r54284 because it broke the build on the Qt buildbot.
Comment 26 Noam Rosenthal 2010-02-03 10:20:34 PST
Any information? build failure log? I thought patches go through the build bot before they go into trunk.
Comment 27 Andras Becsi 2010-02-03 10:26:23 PST
(In reply to comment #26)
> Any information? build failure log? I thought patches go through the build bot
> before they go into trunk.
The Early Warning System does not work for some reason (the box is white, not green).
You can find the buildbots here:
http://build.webkit.org/waterfall

And the error log here:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/6903/steps/compile-webkit/logs/stdio

Unfortunately this log is only kept for one or two days.
Comment 28 Laszlo Gombos 2010-02-03 10:27:58 PST
Line m_sourceMatrix = m_layer.data()->transform(); in GraphicsLayerQt.cpp fails to compile with error:

../../../WebCore/platform/graphics/qt/GraphicsLayerQt.cpp: In member function ‘virtual void WebCore::TransformAnimationQt::updateState(QAbstractAnimation::State, QAbstractAnimation::State)’:
../../../WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:1025: error: no match for ‘operator=’ in ‘((WebCore::TransformAnimationQt*)this)->WebCore::TransformAnimationQt::m_sourceMatrix = QGraphicsItem::transform() const()’
../../../WebCore/platform/graphics/transforms/TransformationMatrix.h:97: note: candidates are: WebCore::TransformationMatrix& WebCore::TransformationMatrix::operator=(const WebCore::TransformationMatrix&)
make[1]: *** [obj/release/GraphicsLayerQt.o] Error 1
Comment 29 Eric Seidel (no email) 2010-02-03 12:34:26 PST
The patch was r+'d before the EWS had a chance to look at it.  It only processes r? patches.

Previously the patch did not apply, thus the EWS bubbles were all purple.  Since Noam is not a committer, it was never tested on the Mac EWS bot.
Comment 30 Eric Seidel (no email) 2010-02-03 12:41:55 PST
The commit-queue itself only tests on Mac Leopard.  The commit-queue also does not look at the results of the EWS bots before attempting commits.  EWS bots are meant to be tools for reviewers.  Laszlo r+'d this while the qt EWS was still white (unprocessed).  Reviewers by *no means* should feel that they have to wait for the bots to review things.  But for patches you might be concerned about building, it it sometimes a good idea to wait for the EWS bots to run through a patch.
Comment 31 Noam Rosenthal 2010-02-03 13:23:51 PST
Created attachment 48064 [details]
Fixed a build issue

Please let the build bots try it before we + the review, though it should hopefully be ok now
Comment 32 Eric Seidel (no email) 2010-02-03 13:46:38 PST
I think the Qt-EWS bot is just really really slow:
http://webkit-commit-queue.appspot.com/
Comment 33 Ariya Hidayat 2010-02-04 08:15:31 PST
Comment on attachment 48064 [details]
Fixed a build issue

LGTM.
Comment 34 WebKit Commit Bot 2010-02-04 08:56:24 PST
Comment on attachment 48064 [details]
Fixed a build issue

Clearing flags on attachment: 48064

Committed r54344: <http://trac.webkit.org/changeset/54344>
Comment 35 WebKit Commit Bot 2010-02-04 08:56:34 PST
All reviewed patches have been landed.  Closing bug.