Bug 32988

Summary: Add RVCT compiler optimization flag Otime
Product: WebKit Reporter: Norbert Leser <norbert.leser>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, hausmann, koshuin, laszlo.gombos, s.mathur, volodimir.burlik, webkit.review.bot, yongjun.zhang, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Bug Depends on: 33082, 33084    
Bug Blocks: 27065    
Description Flags
Patch for rvct -Otime optimization
eric: review-
Updated patch for bug #32988
Updated patch for bug #32988
Update of patch for bug #32988 none

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
Attachment 45565 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/API/JSClassRef.cpp:64:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/API/JSClassRef.cpp:153:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2
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
In general, I think the idea is good. If the focus of this change is to speed up JavaScript execution, than perhaps we should only turn on optimization for JavaScriptCore. Since JavaScriptCore has a small ROM footprint, the impact of the overall QtWebKit ROM footprint would be a lot smaller.

The approach described above is also used for platforms with the gcc compiler (see last few lines of JavaScriptCore.pro).

Norbert, can you update the patch with the suggestions above from Eric and myself and redo the measurements (e.g. SunSpider, ROM and load time impact) ?
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
Created attachment 45707 [details]
Updated patch for bug #32988

According to suggestions, the updated patch compiles JavaScriptCore related code only with -Otime -O3 compiler options.

Since we are dealing with one big library, it is hard to separate JSC from the rest, specifically since there is this name clash of the common config.h file
(i.e., there is no common config header just for JSC). I opted for adding the pragma statement for the optimization flags into runtime/structure.h which appears
to be one of the most common header files used by JSC code.

I still get pretty good performance results (though, about 8% less than with full optimization of all webkit code).
The space increase of the dll (in my comparative builds) is from 4,178 kB to 4,561 kB (the full optimization generates dll with 4,800+ kB).
Code size increase via elftran show from 006edafc to 007d9d8c.

I still need to get data in from QA regression tests with more comparison of startup time etc.

I did not turn the review flag back on yet - before I get more feedback.
Comment 8 Laszlo Gombos 2009-12-31 06:02:44 PST
Does this mean that WebCore/html/canvas/CanvasRenderingContext2D.cpp somehow includes JavaScriptCore/runtime/Structure.h - that does not quite make sense to me. 

If that is the case we should try to find the dependency chain and break it.
Comment 9 Norbert Leser 2009-12-31 06:19:00 PST
(In reply to comment #8)
> Does this mean that WebCore/html/canvas/CanvasRenderingContext2D.cpp somehow
> includes JavaScriptCore/runtime/Structure.h - that does not quite make sense to
> me. 
> If that is the case we should try to find the dependency chain and break it.

I don't disagree that there are structural flaws in the JavaScriptCore code. However, the first thing that I'd like to be fixed is to resolve the implicit dependencies between WebCore and JavaScriptCore. For instance, it is really scary that when compiling webkit, JavaScriptCore picks up the conf.h from WebCore rather than the one defined in JavaScriptCore. The code should really be separated, and JavaScriptCore should have at least one common header for its code.
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
I found a simple way to get around the WebCore build issue - see bug 33082.

I would propose to create a patch just to address the RVCT build problem in JavaScriptCore/API/JSClassRef.cpp without actually changing the build optimization options. 

We can than focus on how to turn on the optimization just for JSC for QtWebKit/Symbian.
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
It seems arbitrary to enable these optimizations in Structure.h. Why did you choose this header file?

Sadly there is currently no way to reliably have compilation flags apply only to files in JavaScriptCore.
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 git@gitorious.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
Created attachment 46320 [details]
Update of patch for bug #32988

We evaluated the memory consumption trade-offs of performance-based optimization of the whole QtWebKit libray versus JavaScriptCore only.
We found that in the case of on-demand paging enabled, there are no noticeable memory use differences.
Also, the initial startup time of webkit applications did not show any differences.
The main difference appears to be the size of the QtWebKit.dll on the device.

Due to these results and because of various comments suggesting to apply the -Otime and -O3 compiler flags for the whole webkit library,
I updated the patch accordingly (actually, reverting to the part of the initial patch that adds these compiler flags to WebCore.pro, conditionally for Symbian).
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