|Summary:||Add RVCT compiler optimization flag Otime|
|Product:||WebKit||Reporter:||Norbert Leser <norbert.leser>|
|Severity:||Normal||CC:||commit-queue, hausmann, koshuin, laszlo.gombos, s.mathur, volodimir.burlik, webkit.review.bot, yongjun.zhang, zalan|
|Version:||528+ (Nightly build)|
|OS:||S60 3rd edition|
|Bug Depends on:||33082, 33084|
Description Norbert Leser 2009-12-28 12:00:54 PST
Created attachment 45565 [details] Patch for rvct -Otime optimization Adding the RVCT compiler (ARM target) optimization flags "-Otime -O3" (Default settings are -Ospace and -O2) significantly increases performance of local JS processing (dramatically better results of V8 and Sunspider tests, for instance). The trade-offs on runtime memory consumption are relatively small, though, text size of dll increases by about 1MB. This compiler flag is set conditionally, for symbian platform. The compiler chokes on , however. It appears that the compiler with -Otime optimization flag tries to optimize out inline new'ed pointers that are passed as arguments. Proposed patch assigns new'ed pointer explicitly outside function call. This should not cause any regression on other platforms. Alternative approach would be to work around this case by using "#pragma Ospace", conditionally for COMPILER(RVCT), but that would add an otherwise unneeded #define.
Comment 1 Laszlo Gombos 2009-12-28 12:19:47 PST
Comment on attachment 45565 [details] Patch for rvct -Otime optimization Clear review flag as I do not think this has been reviewed by a reviewer.
Comment 2 WebKit Review Bot 2009-12-28 12:26:33 PST
Comment 3 Eric Seidel (no email) 2009-12-29 00:11:42 PST
Comment on attachment 45565 [details] Patch for rvct -Otime optimization Tabs will cause this commit to fail. This line will also cause the commit to fail: + No new tests. (OOPS!) You should either list the tests you're affecting, or explain why this change doesn't need tests.
Comment 4 Janne Koskinen 2009-12-29 00:45:04 PST
I'm pretty sure we don't want to have this optimisation on for non-paged 3rd party installation of QtWebkit. For upcoming devices this propably is very viable option to gain performance. 1MB is massive increase in binary size. QtWebkit stands to be slighly below 3MB that is 33%. or did you mean 1MB increase of .text segment? Size of that in my build is ~5.8MB, still 17%. How can you say it is a small impact ? :) Have you tried running this on older devices, what is the impact on load times of the DLL, how much more memory is consumed for loading? I've had issues running debug build of webkit DLL on Nokia N95 due to device running out of memory while trying to load the DLL. Debug size was 8MB (.text 20MB) DLL load consumption is shared with processes, but 1MB extra impact is critically high. "elftran -dump h <file>" will tell you how much is required to load the DLL. So the question boils to how much memory we will allow users to have for their client app.
Comment 5 Laszlo Gombos 2009-12-29 05:11:34 PST
Comment 6 Norbert Leser 2009-12-29 05:23:55 PST
All good suggestions. I will run tests with JS optimization only and create a new patch, once I have the results.
Comment 7 Norbert Leser 2009-12-31 05:40:13 PST
Comment 8 Laszlo Gombos 2009-12-31 06:02:44 PST
Comment 9 Norbert Leser 2009-12-31 06:19:00 PST
Comment 10 Laszlo Gombos 2009-12-31 06:58:34 PST
The issue you referring to is being tracked by bug 31670 and 27551 (https://bugs.webkit.org/show_bug.cgi?id=27551). We might end up delaying this optimization work until 27551 is resolved.
Comment 11 Laszlo Gombos 2009-12-31 08:42:49 PST
Comment 12 Norbert Leser 2009-12-31 09:34:39 PST
Created attachment 45715 [details] Updated patch for bug #32988 Removed compiler issues from this patch. These are being tracked now separately with bugs 33084 and 33082.
Comment 13 Simon Hausmann 2010-01-05 01:07:19 PST
Comment 14 zalan 2010-01-05 02:51:44 PST
any plans to extend -o3 -oTime for the entire qtWebKit codebase for products that can manage the extra memory consumption and all the other side effects? is there any other reason, besides the increased DLL size to not to do it? It is really good to see speed optimized builds for jscore, but real life use cases, including widgets, would benefit a lot more, if the optimization was extended to the rest of the codebase.
Comment 15 Simon Hausmann 2010-01-07 08:46:35 PST
(In reply to comment #12) > Created an attachment (id=45715) [details] > Updated patch for bug #32988 > > Removed compiler issues from this patch. These are being tracked now separately > with bugs 33084 and 33082. This patch is now tracked in staging/4.6/webkit-bug-32988 at firstname.lastname@example.org:+qtwebkit-developers/webkit/qtwebkit.git and was merged into qtwebkit-4.6 with commit 5241d7cda97d86ffac1f390ce33e05469e5dcf31
Comment 16 Norbert Leser 2010-01-11 16:44:48 PST
Comment 17 Vlad Burlik 2010-01-13 12:26:59 PST
The bug in RVCT is not related to passing result of new operation. The case is when local variable (allocated in stack) and passed to inline function by references or as a pointer.
Comment 18 Laszlo Gombos 2010-01-14 19:59:45 PST
Comment on attachment 46320 [details] Update of patch for bug #32988 Looks good to me.
Comment 19 WebKit Commit Bot 2010-01-14 20:20:45 PST
Comment on attachment 46320 [details] Update of patch for bug #32988 Clearing flags on attachment: 46320 Committed r53315: <http://trac.webkit.org/changeset/53315>
Comment 20 WebKit Commit Bot 2010-01-14 20:20:54 PST
All reviewed patches have been landed. Closing bug.
Comment 21 Simon Hausmann 2010-01-15 02:00:12 PST
(In reply to comment #19) > (From update of attachment 46320 [details]) > Clearing flags on attachment: 46320 > > Committed r53315: <http://trac.webkit.org/changeset/53315> Cherry-picked into qtwebkit-4.6 as commit a95c5c0ed2ddb59d3f7b7069967bce8174a7e5b8