Bug 28876 - [Qt] reduce peak memory consumption of text decoding.
Summary: [Qt] reduce peak memory consumption of text decoding.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-09-01 09:00 PDT by Yongjun Zhang
Modified: 2009-10-21 20:06 PDT (History)
5 users (show)

See Also:


Attachments
Cut big input buffer to small buffers to reduce peak memory during decoding. (1.76 KB, patch)
2009-09-01 10:02 PDT, Yongjun Zhang
ariya.hidayat: review-
Details | Formatted Diff | Diff
modified patch as per Ariya's comment. (2.59 KB, patch)
2009-09-16 11:25 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Oops! the previous patch has unnecessary changes to ChangeLog file :) (1.87 KB, patch)
2009-09-16 11:30 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2009-09-01 09:00:30 PDT
Currently, Qt text codec takes the whole input string and decode it into unicode QString, and QString is then converted to WebCore::String for further processing.

QString usually takes twice as much memory as the input string, and copying QString to WebCore::String needs the same amount memory.  As a result, the peak memory consumption of decoding is 5 times as the input string (1x for original input + 2x for QString + 2x for WebCore::String).  That means, for 1MB input text, at least 5 MB RAM is needed to decode it into unicode.
Comment 1 Yongjun Zhang 2009-09-01 10:02:15 PDT
Created attachment 38868 [details]
Cut big input buffer to small buffers to reduce peak memory during decoding.

The fix cuts big input buffer to small buffers and feed them to the decoder; the decoded result is appended into the output buffer. This way, we can save ~40% of memory consumption.  (5x of input buffer to 3x input buffer).
Comment 2 Eric Seidel (no email) 2009-09-03 01:47:50 PDT
This looks OK to me, but Darin Adler is probably the best person to review this (small) change.
Comment 3 Ariya Hidayat 2009-09-03 02:06:32 PDT
Comment on attachment 38868 [details]
Cut big input buffer to small buffers to reduce peak memory during decoding.


> +    // We chop input buffer to smaller buffers to avoid excessive memory consumption
> +    // when the input buffer is big.  This helps reduce peak memory consumption in
> +    // mobile devices where system RAM is limited.
> +    static const int kInputChunkSize = 32 * 1024;

In Qt we don't use Hungarian notation :) You can safely drop "k" prefix there.

> +    const char* buf = bytes;
> +    const char* end = buf + length;
> +    String unicode;
> +
> +    while (buf < end) {
> +        int size = end - buf;
> +        size = qMin(size, kInputChunkSize);
> +        QString decoded = m_codec->toUnicode(buf, size, &m_state);
> +        unicode.append(decoded);
> +        buf += size;
> +    }

While this reduced the peak heap, my concern is the speed. Because we append a small block every time, String needs to grow and reallocate.
Also QTextCode constantly allocates and deallocates 32KB QString.

Can you use Valgrind to see the difference in the memory consumption?

Can we perhaps specify the inputChunkSize, say a small value for Symbian but a larger one for other platforms?

r- until these issues are addressed.
Comment 4 Yongjun Zhang 2009-09-16 11:25:59 PDT
Created attachment 39652 [details]
modified patch as per Ariya's comment.

thanks for the comment, Ariya.

I did memory check with valgrind, the peak memory saving is about 300KB for news.google.com.  I modified the patch to use a small chunk size (32K) for Symbian while set it to 1MB for other platforms.
Comment 5 Yongjun Zhang 2009-09-16 11:30:37 PDT
Created attachment 39653 [details]
Oops!  the previous patch has unnecessary changes to ChangeLog file :)
Comment 6 Kenneth Rohde Christiansen 2009-09-16 14:19:04 PDT
(In reply to comment #4)
> Created an attachment (id=39652) [details]
> modified patch as per Ariya's comment.
> 
> thanks for the comment, Ariya.
> 
> I did memory check with valgrind, the peak memory saving is about 300KB for
> news.google.com.  I modified the patch to use a small chunk size (32K) for
> Symbian while set it to 1MB for other platforms.

I guess a small chunk size is important for most embedded devices, so maybe the test would make sense for ARM in general?
Comment 7 Kenneth Rohde Christiansen 2009-09-16 14:21:25 PDT
I personally would find this useful for other devices that are not Symbian based, like one we're currently using for another Nokia product.
Comment 8 Simon Hausmann 2009-09-25 01:56:26 PDT
Committed r48752: <http://trac.webkit.org/changeset/48752>
Comment 9 Antonio Gomes 2009-10-21 20:06:36 PDT
Comment on attachment 39653 [details]
Oops!  the previous patch has unnecessary changes to ChangeLog file :)

clearing r+ flag, since patch has landed