Bug 104773 - [EFL] Quit debug build without -DSHARED_CORE=ON
Summary: [EFL] Quit debug build without -DSHARED_CORE=ON
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-12 00:01 PST by Halton Huo
Modified: 2013-01-28 12:20 PST (History)
8 users (show)

See Also:


Attachments
Patch (55.04 KB, patch)
2012-12-26 03:02 PST, Halton Huo
no flags Details | Formatted Diff | Diff
Patch (56.04 KB, patch)
2012-12-27 00:52 PST, Halton Huo
no flags Details | Formatted Diff | Diff
Patch (1.41 KB, patch)
2013-01-11 00:29 PST, Halton Huo
no flags Details | Formatted Diff | Diff
Patch (3.38 KB, patch)
2013-01-13 18:42 PST, Halton Huo
no flags Details | Formatted Diff | Diff
Patch (4.92 KB, patch)
2013-01-15 19:10 PST, Halton Huo
no flags Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2013-01-20 18:48 PST, Halton Huo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Halton Huo 2012-12-12 00:01:06 PST
As GTK port did, WebCore of EFL port should be spitted into several library instead of a big one, otherwise the libWebCore.a will be too big to archive. As GTK trunk@137102, the idea is the following:
 - libwebcore_efl contains only the files using efl (was in PlatformEfl.cmake)
 - libwebcore_modules contains only the files under Modules
 - libwebcore_platform contains only the files under platform
 - libwebcore_svg contains only the files under svg, rendering/svg, derived SVG files and other SVG files(there are some svg related files under css loader and platform)
 - libwebcore contains all other the webcore files
Comment 1 Halton Huo 2012-12-19 17:47:39 PST
Iā€˜v made a patch separate libraries as described. Only for non SHARED_CORE build, still has lots sysmbols issues for SHARED_CORE build. Is that acceptable to disable SHARED_CORE build? Just like Gtk port does.
Comment 2 Halton Huo 2012-12-19 17:58:34 PST
Alternatively, I could keep one single libwebcore.so for SHARED_CORE build. Shared core build (libwebcore.so) size won't exceed 4G limitation.

gyuyong and lgombos, I want to hear your opinion.
Comment 3 Gyuyoung Kim 2012-12-20 00:23:26 PST
(In reply to comment #0)
> As GTK port did, WebCore of EFL port should be spitted into several library instead of a big one, otherwise the libWebCore.a will be too big to archive. As GTK trunk@137102, the idea is the following:

r137102 was for chromium gardening patch.

It seems to me below revisions are for splitting into more smaller one.

http://trac.webkit.org/changeset/134864
http://trac.webkit.org/changeset/135538

>  - libwebcore_efl contains only the files using efl (was in PlatformEfl.cmake)
>  - libwebcore_modules contains only the files under Modules
>  - libwebcore_platform contains only the files under platform
>  - libwebcore_svg contains only the files under svg, rendering/svg, derived SVG files and other SVG files(there are some svg related files under css loader and platform)
>  - libwebcore contains all other the webcore files

Basically, I don't wanna object to divide into small libraries. But, AFAIK, SHARED_CORE build option is to reduce linking time as well. Why not adjust this splitting into existing SHARED_CORE option ? BTW, could you share how much linking time we can reduce when using this dividing library ?
Comment 4 Halton Huo 2012-12-21 00:07:40 PST
(In reply to comment #3)
> Basically, I don't wanna object to divide into small libraries. But, AFAIK, SHARED_CORE build option is to reduce linking time as well. Why not adjust this splitting into existing SHARED_CORE option ? BTW, could you share how much linking time we can reduce when using this dividing library ?

My intent for this bug is to resolving libwebcore_efl.a is too big(over 4g) to archive issue. Note, this is only for non shared_core build. For shared_core build, size of libwebcore_efl.so is okay (1.2g). Again, this is only for Debug build, Release build does not has this issue. Following table can make things more clear.

Debug(non-shared): libwebcore_efl.a over 4g
Debug(shared): libwebcore_efl.so 1.2g
Release(non-shared): libwebcore_efl.a 119m
Release(shared): libwebcore_efl.so 1.2g 47m

So split libwebcore_efl(shared or non-shared) won't speed up linking time.

But I do find some linking issues for debug non-shared build. My understanding only  libewebkit.so (or libewebkit2.so) should be linked to targets MiniBrowser, EWebLauncher, DumpRenderTree and WebKitTestRunner, test_ewk_*, test_ewk2_* and test_webkit2_api_*. The static libraries the wtf_efl.a, javascript_efl.a and libwebcore_efl.a are still listed in link.txt. That may cause the linking time too long.
Comment 5 Laszlo Gombos 2012-12-25 18:33:56 PST
(In reply to comment #4)
> (In reply to comment #3)

> My intent for this bug is to resolving libwebcore_efl.a is too big(over 4g) to archive issue. Again, this is only for Debug build, Release build does not has this issue.

Link to a related discussion: https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/1043507 .

I would like to understand better when this error condition happens for Debug builds. Is this only for 32 bit systems or this impacts 64 bit systems as well ?
Comment 6 Halton Huo 2012-12-25 18:50:27 PST
(In reply to comment #5)
> Link to a related discussion: https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/1043507 .
> 
> I would like to understand better when this error condition happens for Debug builds. Is this only for 32 bit systems or this impacts 64 bit systems as well ?

I'm on 64-bit. See https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/1043507/comments/16 also 64-bit. 

According my investigation on comment #4, I'm now re-factoring my patch that only separate libraries for non-shared build.
Comment 7 Laszlo Gombos 2012-12-25 23:28:20 PST
(In reply to comment #6)
> (In reply to comment #5)

> According my investigation on comment #4, I'm now re-factoring my patch that only separate libraries for non-shared build.

Generally we should try to minimize the number of build configurations supported as every new way of building from the source adds complexity.

1./ Can we separate out the debug info into separate archive(s) ? Would this solve the problem ?  http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html 

2./ I wonder if we should simply only support Debug builds with SHARED_CORE build. It certainly allows people for debug and it is not an issue for creating products that are going out with release builds.
Comment 8 Halton Huo 2012-12-26 00:14:30 PST
(In reply to comment #7)
> Generally we should try to minimize the number of build configurations supported as every new way of building from the source adds complexity.
> 
> 1./ Can we separate out the debug info into separate archive(s) ? Would this solve the problem ?  http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html 

I do not think this will resolve the non-SHARED_CORE and debug build issue. Because the libwebcore_efl.a need to be archived out before it get "stripped". Correct me if I am wrong.
> 
> 2./ I wonder if we should simply only support Debug builds with SHARED_CORE build. It certainly allows people for debug and it is not an issue for creating products that are going out with release builds.

+1 for solution, it is much more easier that what I'm doing and small change.

So what should build do when debug build but no "-DSHARED_CORE=ON"?
Option 1: Stop build and say debug build need set -DSHARED_CORE=ON
Option 2: Set(SHARED_CORE ON) and continue building.

Your opinion?
Comment 9 Halton Huo 2012-12-26 03:02:48 PST
Created attachment 180737 [details]
Patch
Comment 10 Halton Huo 2012-12-26 03:15:36 PST
(In reply to comment #9)
> Created an attachment (id=180737) [details]
> Patch
This patch will build separated libraries when SHARED_CORE is OFF, upload here is just for a reference and patch verification for other porting. 

If all agree goes with solution in comment #8, i will offer a new patch.
Comment 11 Halton Huo 2012-12-27 00:52:55 PST
Created attachment 180777 [details]
Patch
Comment 12 Halton Huo 2012-12-27 01:04:23 PST
(In reply to comment #11)
> Created an attachment (id=180777) [details]
> Patch
This patch is verified with/without -DSHARED_CORE=ON, debug and release build, So far so good. I think it is ready for review. Again, If all agree goes with solution in comment #8, i will offer a different patch.
Comment 13 Gyuyoung Kim 2012-12-27 21:12:41 PST
(In reply to comment #8)

> So what should build do when debug build but no "-DSHARED_CORE=ON"?
> Option 1: Stop build and say debug build need set -DSHARED_CORE=ON

I prefer option 1. But, I'm not sure if we should stop to build when disabling SHARED_CORE. Or, we may ask if you wanna build without SHARED_CORE.
Comment 14 Halton Huo 2012-12-27 22:37:48 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=180777) [details] [details]
> > Patch
> This patch is verified with/without -DSHARED_CORE=ON, debug and release build, So far so good. I think it is ready for review. Again, If all agree goes with solution in comment #8, i will offer a different patch.

(In reply to comment #13)
> (In reply to comment #8)
> 
> > So what should build do when debug build but no "-DSHARED_CORE=ON"?
> > Option 1: Stop build and say debug build need set -DSHARED_CORE=ON
> 
> I prefer option 1. But, I'm not sure if we should stop to build when disabling SHARED_CORE. Or, we may ask if you wanna build without SHARED_CORE.

What about Option 3 - Only separate libwebcore.a when SHARED_CORE build? Which is functionally working according my testing and ready for review.
Comment 15 Halton Huo 2013-01-11 00:29:38 PST
Created attachment 182280 [details]
Patch
Comment 16 Halton Huo 2013-01-11 00:31:09 PST
(In reply to comment #15)
> Created an attachment (id=182280) [details]
> Patch

This patch goes as "Option 1: Stop build and say debug build need set -DSHARED_CORE=ON" that Gyuyoung prefers. Please review.
Comment 17 Laszlo Gombos 2013-01-12 21:25:19 PST
Comment on attachment 182280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182280&action=review

r- to make sure that this patch is only applied to the EFL port.

> CMakeLists.txt:11
> +if (${CMAKE_BUILD_TYPE} MATCHES "^[Dd][Ee][Bb][Uu][Gg]$" AND NOT SHARED_CORE)

As far as I can tell this rule is only needed for the EFL port, but this line will be evaluated for all ports. Can we instead move this logic to OptionsEfl.cmake ?

Can we use the following line instead before checking for the build type
string(TOLOWER ${CMAKE_BUILD_TYPE} CMAKE_BUILD_TYPE)

> CMakeLists.txt:12
> +    message(FATAL_ERROR "Debug build need set -DSHARED_CORE=ON")

How about 'Turn on the SHARED_CORE flag to make a debug build - e.g. build-webkit --efl --debug --cmakeargs="-DSHARED_CORE=ON" ' as most people use build-webkit.

My preference is option 2 as the only way to make a debug build and the moment is using this option, so I do not see a problem automatically setting it for debug builds, but both option 1 and option 2 is better than what we have now so I do not have a strong preference.
Comment 18 Halton Huo 2013-01-13 18:42:13 PST
Created attachment 182499 [details]
Patch
Comment 19 Halton Huo 2013-01-13 20:02:37 PST
(In reply to comment #18)
> Created an attachment (id=182499) [details]
> Patch

Laszlo, this reworked patch still goes with option 1 I get Gyuyoung's +1. 

Actually, I did some trying with option 2, but set(SHARED_CORE ON) won't affect value in CMakeCache.txt, if we goes that way, I need continue to work.
Comment 20 Laszlo Gombos 2013-01-15 08:58:13 PST
Comment on attachment 182499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182499&action=review

r- for the regressions. The approach looks good to me.

> CMakeLists.txt:10
> +string(TOLOWER ${CMAKE_BUILD_TYPE} CMAKE_BUILD_TYPE)

I was expecting this line to be placed in OptionsEfl.cmake. I think this approach can work as well but than you need to check and correct all the places where CMAKE_BUILD_TYPE is used already.

This would cause regression in the following places (and potentially some more):

OptionsCommon.cmake:if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)" AND "${CMAKE_BUILD_TYPE}" STREQUAL "Debug")

OptionsEfl.cmake:if (CMAKE_BUILD_TYPE STREQUAL Release AND CMAKE_COMPILER_IS_GNUCC AND UNIX AND NOT APPLE)
Comment 21 Halton Huo 2013-01-15 19:10:47 PST
Created attachment 182904 [details]
Patch
Comment 22 Halton Huo 2013-01-15 19:12:39 PST
(In reply to comment #20)
> I was expecting this line to be placed in OptionsEfl.cmake. I think this approach can work as well but than you need to check and correct all the places where CMAKE_BUILD_TYPE is used already.
> 
> This would cause regression in the following places (and potentially some more):
> 
> OptionsCommon.cmake:if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)" AND "${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
> 
> OptionsEfl.cmake:if (CMAKE_BUILD_TYPE STREQUAL Release AND CMAKE_COMPILER_IS_GNUCC AND UNIX AND NOT APPLE)

Patch updated, I searched all the code base, you've find all places that ${CMAKE_BUILD_TYPE} is used, thanks for finding it.
Comment 23 Laszlo Gombos 2013-01-18 09:49:52 PST
Comment on attachment 182904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182904&action=review

> CMakeLists.txt:144
>  include(Options${PORT})
>  
> +if (SHARED_CORE)
> +    set(JavaScriptCore_LIBRARY_TYPE SHARED)
> +    set(WebCore_LIBRARY_TYPE SHARED)
> +else ()
> +    set(JavaScriptCore_LIBRARY_TYPE STATIC)
> +    set(WebCore_LIBRARY_TYPE STATIC)
> +endif ()
> +
> +set(WebKit_LIBRARY_TYPE SHARED)
> +set(WebKit2_LIBRARY_TYPE SHARED)
> +set(WebCoreTestSupport_LIBRARY_TYPE STATIC)
> +

Is this change still needed ? Options${PORT} should have precedence over CMakeList.txt. This might have an impact on the WinCE port that has a "set(WebKit_LIBRARY_TYPE STATIC)" rule that needs to take precedence.

Sorry that I did not catch this in my previous review.
Comment 24 Halton Huo 2013-01-20 18:48:40 PST
Created attachment 183697 [details]
Patch
Comment 25 Halton Huo 2013-01-20 18:50:08 PST
(In reply to comment #24)
> Created an attachment (id=183697) [details]
> Patch

Update to resolve Laszlo's comment 23.
Comment 26 Laszlo Gombos 2013-01-24 05:56:29 PST
Comment on attachment 183697 [details]
Patch

r=me. Thanks !
Comment 27 WebKit Review Bot 2013-01-28 12:20:45 PST
Comment on attachment 183697 [details]
Patch

Clearing flags on attachment: 183697

Committed r140990: <http://trac.webkit.org/changeset/140990>
Comment 28 WebKit Review Bot 2013-01-28 12:20:51 PST
All reviewed patches have been landed.  Closing bug.