Bug 41480

Summary: [Qt][Symbian] Variable max heap size between target/emulator
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: PlatformAssignee: Siddharth Mathur <s.mathur>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hausmann, koshuin, laszlo.gombos, s.mathur
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Make max heap variable for ARM/emulator. Also use paging
none
increased emulator limit, and explicitly used pageddata and pagedcode directives
none
updated patch with paging config removed
none
Correct screw up in previous patch where older TARGET.EPOCHEAPSIZE wasn't deleted none

Description Laszlo Gombos 2010-07-01 12:50:52 PDT
Siddharth is telling me that on the simulator 96 MB for max heapsize it to high. The limit in WebKit.pri should be different for WINSCW than for target. Patches welcome.
Comment 1 Siddharth Mathur 2010-07-01 13:10:43 PDT
(In reply to comment #0)
> Siddharth is telling me that on the simulator 96 MB for max heapsize it to high. The limit in WebKit.pri should be different for WINSCW than for target. Patches welcome.

How about I kill many birds with one stone, and make a QMake function that _all_  Symbian EXEs can use to set their heap sizes? Also turn on code paging and data paging automagically?
Comment 2 Siddharth Mathur 2010-07-01 13:34:57 PDT
I rephrased the title to better reflect that I am doing.
Comment 3 Laszlo Gombos 2010-07-01 14:20:31 PDT
Siddharth, we're already consistent I think as the HEAPSIZE is set currently for all .exe's that are built from the WebKit tree (WebKit.pri), including tests and DumpRenderTree exe's.

Also note that I do not know what are the Symbian defaults for code paging and data paging.
Comment 4 Siddharth Mathur 2010-07-01 15:02:20 PDT
Created attachment 60293 [details]
Make max heap variable for ARM/emulator. Also use paging

Make max heap variable for ARM, emulator so that developers don't see jammed emulator. Also use paging. OSes that don't support the bit flag in the EXE will silently ignore it. 
Verified using QTestBrowser and other test EXEs that directives show up in MMP and ARMV5 UREL shows correct values using elf2e32
Comment 5 Laszlo Gombos 2010-07-01 19:55:26 PDT
Comment on attachment 60293 [details]
Make max heap variable for ARM/emulator. Also use paging

Looks good - 2 minor issues:

1./ Can you make a reference to the bugzilla bug in the ChangeLog (prepare-ChangeLog --bug <bugid>will do that for you)? 

2./ Patch does not seems to apply on the bot and has tab character in it
Comment 6 Janne Koskinen 2010-07-02 00:17:23 PDT
Hi,

PAGED keyword is deprecated and should not be used with SBSv2. Instead you should be using codepaging and datapaging keywords. These keywords also come from Qt and are added to all Qt DLLs so you don't need to add them separately.

16MB for emulator? please, that will not run anything. If you are having troubles getting more for emulator you need to tweak your environment. max heapsize defaults to 20MB in Qt which is way too low for webkit apps.
I've used 48MB in my test apps as that is the roughly the limit of my test phone and with that amount never had issues of emulator not starting but have had issues of running out of max app heap. I'd set it to 64MB and learn how to get more out of your environment.

My machine has 3GB of RAM so you might have issues if you have less than that.
Closing other programs esp. webbrowser before running emulator helps on releasing commited memory.
Comment 7 Janne Koskinen 2010-07-02 00:39:22 PDT
This actually raises a question. Have you modified epoc.ini ?
I have set MegabytesOfFreeMemory 256
and test apps as 48MB...

You do realise if you set a separate limit to emulator and target you will get a very different behaviour which leads to hundreds of 'bugs' in the emulator env which can't be reproduced on HW. Or worse crashes in the emulator might hide the real bugs that only appear once we have enough memory and that is in the HW.

Users will not understand this and we end up answering a lot of forum questions and spending lot of time on debugging bogus reports. Ofc. this is issue of the emulator which we can't do much about except drop the support for good.

From my perspective the app not starting at all is much easier to explain than myriad crashes.
Comment 8 Siddharth Mathur 2010-07-02 08:32:30 PDT
Created attachment 60369 [details]
increased emulator limit, and explicitly used pageddata and pagedcode directives
Comment 9 Siddharth Mathur 2010-07-15 08:49:44 PDT
(In reply to comment #8)
> Created an attachment (id=60369) [details]
> increased emulator limit, and explicitly used pageddata and pagedcode directives

Janne, 
Did I address your concerns the other day over IRC? I upped the emulator limit to the higher value we discussed. Hopefully, "normal" Qt users as well as QtProd users will get more out-of-the-box usable values with this patch. 
Also, the engineers trying to get DRT to work on N8 will also see fewer crashes because the DRT binary now gets data paging by default.
Comment 10 Simon Hausmann 2010-07-22 07:01:35 PDT
(In reply to comment #6)
> PAGED keyword is deprecated and should not be used with SBSv2. Instead you should be using codepaging and datapaging keywords. These keywords also come from Qt and are added to all Qt DLLs so you don't need to add them separately.

Mahesh, what about Janne's comment here?
Comment 11 Siddharth Mathur 2010-07-22 07:11:17 PDT
(In reply to comment #10)
> (In reply to comment #6)
> > PAGED keyword is deprecated and should not be used with SBSv2. Instead you should be using codepaging and datapaging keywords. These keywords also come from Qt and are added to all Qt DLLs so you don't need to add them separately.
> 
> Mahesh, what about Janne's comment here?

My patch uses pageddata and pagedcode keywords, which are the more fine grained way of achieving the same effect. Janne was OK with this in my IRC chat with him, but he is on vacation it seems. 

Note that if your compiling QtWebkit/Symbian on an older toolchain say ABLD on 3.2 or 5.0, the tools will ignore the pageddata keyword with a warning printed about "unidentified keyword" . That's OK. 

Hopefully, with this patch DRT can run better on Symbian^3 due to use of data paging.
Comment 12 Siddharth Mathur 2010-08-02 13:51:05 PDT
Removing paging part from bug description, as we will let Qt-Symbian's configuration handle that part.
Comment 13 Siddharth Mathur 2010-08-02 14:22:41 PDT
Created attachment 63260 [details]
updated patch with paging config removed
Comment 14 Siddharth Mathur 2010-08-02 15:01:14 PDT
Created attachment 63264 [details]
Correct screw up in previous patch where older TARGET.EPOCHEAPSIZE wasn't deleted

Verified that the following shows up OK in \WebKitTools\QtTestBrowser\QtTestBrowser_0xA000E543.mmp and WebKitTools\DumpRenderTree\qt\DumpRenderTree_0xE5192995.mmp (for example). 

#ifdef WINSCW
EPOCHEAPSIZE 0x40000 0x2000000 // Min 256kB, Max 32MB
#else
EPOCHEAPSIZE 0x40000 0x6000000 // Min 256kB, Max 96MB
#endif
Comment 15 Laszlo Gombos 2010-08-02 18:11:37 PDT
Comment on attachment 63264 [details]
Correct screw up in previous patch where older TARGET.EPOCHEAPSIZE wasn't deleted

Looks good to me now that the explicit paging rules are removed. r+. Thanks.
Comment 16 WebKit Commit Bot 2010-08-04 11:09:16 PDT
Comment on attachment 63264 [details]
Correct screw up in previous patch where older TARGET.EPOCHEAPSIZE wasn't deleted

Clearing flags on attachment: 63264

Committed r64657: <http://trac.webkit.org/changeset/64657>
Comment 17 WebKit Commit Bot 2010-08-04 11:09:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Laszlo Gombos 2010-08-19 05:48:34 PDT
This patch introduced a new build warning we should look into getting rid of it..

Warning: Restricted statements detected in MMP_RULES:
         (EPOCHEAPSIZE)
         Use corresponding qmake variable(s) instead.
Comment 19 Siddharth Mathur 2010-08-19 07:14:27 PDT
(In reply to comment #18)
> This patch introduced a new build warning we should look into getting rid of it..
> 
> Warning: Restricted statements detected in MMP_RULES:
>          (EPOCHEAPSIZE)
>          Use corresponding qmake variable(s) instead.

That warning is qmake's way of telling us that a normal user shouldn't have a need to use anything except TARGET.EPOCHEAPSIZE directive. Since we can't achieve conditional heap sizes with simply TARGET.EPOCHEAPSIZE, this warning is OK to incur. 
If you can verify that your generated webcore_<UID3>.mmp has the correct syntax, then things are OK.
Comment 20 Simon Hausmann 2010-10-20 02:17:43 PDT
Revision r64657 cherry-picked into qtwebkit-2.1 with commit 80daa72 <http://gitorious.org/webkit/qtwebkit/commit/80daa72>
Comment 21 Simon Hausmann 2010-10-20 02:18:31 PDT
re-opening as it was before the cherry-pick
Comment 22 Siddharth Mathur 2010-10-20 07:31:23 PDT
I don't think any action is required on this. Hence resolving.