Bug 54887 – [Qt] High memory usage for accelerating compositing html layers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Misha]

Qt accelerating compositing uses QPixmapCache. When web site has a big accelerating compositing layer on devices with low memory we can reach the QPixmapCache limit very fast. In this case pixmap is dropped from QPixmapCache and getting re-allocated on every paint. This makes performance very bad. 
Increasing  QPixmapCache limit wouldn't help in this case since we run out of graphics memory. In the same time those html layers are quite simple and render fast without any cache on graphics accelerated hardware. 
I propose to disable html layer cache in this case. It will still allow to remove dynamic content from tiled backing store with AC and keep AC memory requirements low.

[Misha]

Created attachment 83172 [details]
patch to address the issue

Patch to address the issue with hight memory usage for AC

[WebKit Review Bot]

Attachment 83172 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:210:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:210:  The parameter name "region" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:648:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebKit/qt/Api/qwebpage.cpp:1177:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebKit/qt/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 83172 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7935953

[Early Warning System Bot]

Attachment 83172 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7935954

[Build Bot]

Attachment 83172 [details] did not build on win:
Build output: http://queues.webkit.org/results/7938872

[Misha]

Created attachment 83188 [details]
addressed style and build issues

[Andreas Kling]

Comment on attachment 83188 [details]
addressed style and build issues

This patch contains only ChangeLog.

[Benjamin Poulain]

What is the point of accelerated compositing in this case? What do you think is "accelerated"?

[Noam Rosenthal]

(In reply to comment #8)
> What is the point of accelerated compositing in this case? What do you think is "accelerated"?
I think the point here is that the AC layers are not tiled.

[Noam Rosenthal]

(In reply to comment #0)
> Qt accelerating compositing uses QPixmapCache. When web site has a big accelerating compositing layer on devices with low memory we can reach the QPixmapCache limit very fast. In this case pixmap is dropped from QPixmapCache and getting re-allocated on every paint. This makes performance very bad. 
> Increasing  QPixmapCache limit wouldn't help in this case since we run out of graphics memory. In the same time those html layers are quite simple and render fast without any cache on graphics accelerated hardware.

Which HTML layers? Which websites? Which platform?
Does this mean that 3D & animations run well on this platform even when this cache is turned off? (which effectively makes AC unaccelerated)

[Viatcheslav Ostapenko]

(In reply to comment #10)
> (In reply to comment #0)
> > Qt accelerating compositing uses QPixmapCache. When web site has a big accelerating compositing layer on devices with low memory we can reach the QPixmapCache limit very fast. In this case pixmap is dropped from QPixmapCache and getting re-allocated on every paint. This makes performance very bad. 
> > Increasing  QPixmapCache limit wouldn't help in this case since we run out of graphics memory. In the same time those html layers are quite simple and render fast without any cache on graphics accelerated hardware.
> 
> Which HTML layers? 

GraphicsLayerQtImpl::HTMLContentType

> Which websites? 

For example, iphone versions of google maps, or iphone gmail.

> Which platform?

Any platform with low RAM (CPU or GPU).

> Does this mean that 3D & animations run well on this platform even when this cache is turned off? (which effectively makes AC unaccelerated)

This is totally unrelated.
3D, animations, media are layers of different type. They do not have cache by default and not affected by this.

This patch about html layers which have full size QPixmap cache by default or create another tiled layer and cause RAM overhead which is quite big.

[Viatcheslav Ostapenko]

(In reply to comment #8)
> What is the point of accelerated compositing in this case? 

Removing dynamic content from tiled backing store.

> What do you think is "accelerated"?

Animated images, plugins and etc.
Just everything as it is, but html layers will not have pixmap cache.

[Misha]

Created attachment 83315 [details]
actual patch

[Build Bot]

Attachment 83315 [details] did not build on win:
Build output: http://queues.webkit.org/results/7942327

[Misha]

Created attachment 83327 [details]
win build fix

[Build Bot]

Attachment 83327 [details] did not build on win:
Build output: http://queues.webkit.org/results/7947244

[Misha]

Created attachment 83375 [details]
new patch

As discussed on irc changes are kept only in GraphicsLayerQt.cpp as they are Qt specific. If QPixmapCache limit is two small (2048Kb) them QPixmapCache is not used when rendering HTMLContentType layers.

[Noam Rosenthal]

(In reply to comment #17)
> Created an attachment (id=83375) [details]
> new patch

LGTM, though I'm not a reviewer.

[Andreas Kling]

Comment on attachment 83375 [details]
new patch

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

> Source/WebCore/ChangeLog:5
> +        [Qt] Dont use QPixmapCache if  QPixmapCache::cacheLimit() is too small 

Double spaces after 'if'.

> Source/WebCore/ChangeLog:6
> +(2048Kb for now).

Should be indented with the line above.

> Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:57
> +#define PIXMAP_CACHE_USE_THRESHOLD 2048

We try to avoid using #define for constants.
static const int gMinimumPixmapCacheLimit = 2048;

> Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:213
> +    void drawLayerContent(QPainter&, QRegion&);

QPainter& -> QPainter*

> Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:649
> +                QRegion region(option->exposedRect.toRect());
> +                drawLayerContent(*painter, region);

We shouldn't be clipping to a region if we know it's always a rect.
Change drawLayerContent() to take a const QRect& instead.

> Source/WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:651
> +

Unnecessary empty line.

[Misha]

Created attachment 83493 [details]
Fixed style and clip region

Addressed  issues in Comment #19

[Andreas Kling]

Comment on attachment 83493 [details]
Fixed style and clip region

r=me

[WebKit Commit Bot]

Comment on attachment 83493 [details]
Fixed style and clip region

Clearing flags on attachment: 83493

Committed r79595: <http://trac.webkit.org/changeset/79595>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79595 might have broken GTK Linux 32-bit Release

[Ademar Reis]

Revision r79595 cherry-picked into qtwebkit-2.1.x with commit 12a2bcc <http://gitorious.org/webkit/qtwebkit/commit/12a2bcc>