Bug 34062 – [Qt] Tuning and optimizations to GraphicsLayerQt

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

[Noam Rosenthal]

Tune the caching methodologies and flow of GraphicsLayerQt for better paint performance

[Noam Rosenthal]

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.

[Ariya Hidayat]

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!

[Noam Rosenthal]

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?

[Ariya Hidayat]

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

[Noam Rosenthal]

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.

[Ariya Hidayat]

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.

[Noam Rosenthal]

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? :)

[Noam Rosenthal]

Thanks Kenneth!

[WebKit Commit Bot]

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

[Noam Rosenthal]

Interesting... someone needs to change the Reviewed-by thingy in the changelogs?

[Laszlo Gombos]

Noam, I think the failure comes from " No new tests. (OOPS!)" in the ChangeLog. That is something you should be able to fix.

[Noam Rosenthal]

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.

[Kenneth Rohde Christiansen]

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;

[Eric Seidel (no email)]

(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

[Noam Rosenthal]

Created attachment 47973 [details]
Performance tuning to GraphicsLayerQt: Some style fixes + correct changelog

Absolutely right - my bad.
Hopefully this patch should do it.

[WebKit Commit Bot]

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

[Noam Rosenthal]

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

[Laszlo Gombos]

Noam, I think you would need to merge with WebKit trunk, resolve the conflicts in GraphicsLayerQt.cpp and upload the patch again.

[Noam Rosenthal]

yes, that's what I just did with the new patch :)

[WebKit Commit Bot]

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

[Noam Rosenthal]

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

[Laszlo Gombos]

Comment on attachment 48024 [details]
re-applying, uploaded wrong file

This time it looks like the patch applies cleanly.

[WebKit Commit Bot]

Comment on attachment 48024 [details]
re-applying, uploaded wrong file

Clearing flags on attachment: 48024

Committed r54281: <http://trac.webkit.org/changeset/54281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andras Becsi]

Unfortunately we had to roll-out the patch in r54284 because it broke the build on the Qt buildbot.

[Noam Rosenthal]

Any information? build failure log? I thought patches go through the build bot before they go into trunk.

[Andras Becsi]

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

[Laszlo Gombos]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Noam Rosenthal]

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

[Eric Seidel (no email)]

I think the Qt-EWS bot is just really really slow:
http://webkit-commit-queue.appspot.com/

[Ariya Hidayat]

Comment on attachment 48064 [details]
Fixed a build issue

LGTM.

[WebKit Commit Bot]

Comment on attachment 48064 [details]
Fixed a build issue

Clearing flags on attachment: 48064

Committed r54344: <http://trac.webkit.org/changeset/54344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.