Bug 32988

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.

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 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.

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.

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) ?

All good suggestions. I will run tests with JS optimization only and create a new patch, once I have the results.

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.

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.

(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.

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.

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.

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.

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.

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.

(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

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).

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 on attachment 46320 [details] Update of patch for bug #32988 Looks good to me.

Comment on attachment 46320 [details] Update of patch for bug #32988 Clearing flags on attachment: 46320 Committed r53315: <http://trac.webkit.org/changeset/53315>

All reviewed patches have been landed. Closing bug.

(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