Bug 54887 - [Qt] High memory usage for accelerating compositing html layers
Summary: [Qt] High memory usage for accelerating compositing html layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P3 Normal
Assignee: Misha
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-02-21 09:23 PST by Misha
Modified: 2011-02-25 12:03 PST (History)
12 users (show)

See Also:


Attachments
patch to address the issue (10.28 KB, patch)
2011-02-21 09:33 PST, Misha
no flags Details | Formatted Diff | Diff
addressed style and build issues (1.33 KB, patch)
2011-02-21 10:45 PST, Misha
no flags Details | Formatted Diff | Diff
actual patch (10.00 KB, patch)
2011-02-22 07:52 PST, Misha
no flags Details | Formatted Diff | Diff
win build fix (10.65 KB, patch)
2011-02-22 09:03 PST, Misha
no flags Details | Formatted Diff | Diff
new patch (4.33 KB, patch)
2011-02-22 13:12 PST, Misha
akling: review-
Details | Formatted Diff | Diff
Fixed style and clip region (4.44 KB, patch)
2011-02-23 09:40 PST, Misha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Misha 2011-02-21 09:23:17 PST
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.
Comment 1 Misha 2011-02-21 09:33:52 PST
Created attachment 83172 [details]
patch to address the issue

Patch to address the issue with hight memory usage for AC
Comment 2 WebKit Review Bot 2011-02-21 09:34:47 PST
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.
Comment 3 WebKit Review Bot 2011-02-21 09:45:04 PST
Attachment 83172 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7935953
Comment 4 Early Warning System Bot 2011-02-21 09:47:03 PST
Attachment 83172 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7935954
Comment 5 Build Bot 2011-02-21 10:01:16 PST
Attachment 83172 [details] did not build on win:
Build output: http://queues.webkit.org/results/7938872
Comment 6 Misha 2011-02-21 10:45:08 PST
Created attachment 83188 [details]
addressed style and build issues
Comment 7 Andreas Kling 2011-02-22 01:57:47 PST
Comment on attachment 83188 [details]
addressed style and build issues

This patch contains only ChangeLog.
Comment 8 Benjamin Poulain 2011-02-22 02:04:52 PST
What is the point of accelerated compositing in this case? What do you think is "accelerated"?
Comment 9 Noam Rosenthal 2011-02-22 05:57:23 PST
(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.
Comment 10 Noam Rosenthal 2011-02-22 06:03:55 PST
(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)
Comment 11 Viatcheslav Ostapenko 2011-02-22 07:49:01 PST
(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.
Comment 12 Viatcheslav Ostapenko 2011-02-22 07:52:15 PST
(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.
Comment 13 Misha 2011-02-22 07:52:59 PST
Created attachment 83315 [details]
actual patch
Comment 14 Build Bot 2011-02-22 08:21:29 PST
Attachment 83315 [details] did not build on win:
Build output: http://queues.webkit.org/results/7942327
Comment 15 Misha 2011-02-22 09:03:56 PST
Created attachment 83327 [details]
win build fix
Comment 16 Build Bot 2011-02-22 09:30:56 PST
Attachment 83327 [details] did not build on win:
Build output: http://queues.webkit.org/results/7947244
Comment 17 Misha 2011-02-22 13:12:22 PST
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.
Comment 18 Noam Rosenthal 2011-02-22 14:02:18 PST
(In reply to comment #17)
> Created an attachment (id=83375) [details]
> new patch

LGTM, though I'm not a reviewer.
Comment 19 Andreas Kling 2011-02-22 14:12:01 PST
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.
Comment 20 Misha 2011-02-23 09:40:05 PST
Created attachment 83493 [details]
Fixed style and clip region

Addressed  issues in Comment #19
Comment 21 Andreas Kling 2011-02-24 07:43:05 PST
Comment on attachment 83493 [details]
Fixed style and clip region

r=me
Comment 22 WebKit Commit Bot 2011-02-24 11:32:15 PST
Comment on attachment 83493 [details]
Fixed style and clip region

Clearing flags on attachment: 83493

Committed r79595: <http://trac.webkit.org/changeset/79595>
Comment 23 WebKit Commit Bot 2011-02-24 11:32:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Review Bot 2011-02-24 15:59:30 PST
http://trac.webkit.org/changeset/79595 might have broken GTK Linux 32-bit Release
Comment 25 Ademar Reis 2011-02-25 12:03:17 PST
Revision r79595 cherry-picked into qtwebkit-2.1.x with commit 12a2bcc <http://gitorious.org/webkit/qtwebkit/commit/12a2bcc>