Bug 104773 – [EFL] Quit debug build without -DSHARED_CORE=ON

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Halton Huo]

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

[Halton Huo]

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.

[Halton Huo]

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.

[Gyuyoung Kim]

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

[Halton Huo]

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

[Laszlo Gombos]

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

[Halton Huo]

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

[Laszlo Gombos]

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

[Halton Huo]

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

[Halton Huo]

Created attachment 180737 [details]
Patch

[Halton Huo]

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

[Halton Huo]

Created attachment 180777 [details]
Patch

[Halton Huo]

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

[Gyuyoung Kim]

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

[Halton Huo]

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

[Halton Huo]

Created attachment 182280 [details]
Patch

[Halton Huo]

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

[Laszlo Gombos]

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.

[Halton Huo]

Created attachment 182499 [details]
Patch

[Halton Huo]

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

[Laszlo Gombos]

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)

[Halton Huo]

Created attachment 182904 [details]
Patch

[Halton Huo]

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

[Laszlo Gombos]

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.

[Halton Huo]

Created attachment 183697 [details]
Patch

[Halton Huo]

(In reply to comment #24)
> Created an attachment (id=183697) [details]
> Patch

Update to resolve Laszlo's comment 23.

[Laszlo Gombos]

Comment on attachment 183697 [details]
Patch

r=me. Thanks !

[WebKit Review Bot]

Comment on attachment 183697 [details]
Patch

Clearing flags on attachment: 183697

Committed r140990: <http://trac.webkit.org/changeset/140990>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.