| Summary: | [CMake] Use ld.gold if it is available to speedup builds | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||
| Component: | Tools / Tests | Assignee: | Csaba Osztrogonác <ossy> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | berto, cgarcia, clopez, commit-queue, gyuyoung.kim, mrobinson, ossy, rakuco, ryuan.choi, sergio | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 137818 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Csaba Osztrogonác
2014-10-22 04:36:18 PDT
Using ld.gold decreased the incremental build time from 1m51s to 50s when CMake always runs. After applying the optimization from bug137949 not to run CMake always, the build time is only 22s instead of the original 1m51s. Created attachment 240267 [details]
WIP patch
WIP patch, added a platform independent USE_GOLD_LINKER option, disabled by default. It can be enabled with build-webkit --cmakeargs="-DUSE_GOLD_LINKER=ON"
cc-ing GTK folks too, maybe you are interested in it too. (The patch is based on your great DEBUG_FISSION option) The question is if we should make using ld.gold default or not. In this case I meant it can be disabled manually or the config could disable it automatically if it isn't available. (with or without warning the developer) Or do you think if using gold linker should be disabled by default for some reason, and should be an opt-in option? (In reply to comment #3) > cc-ing GTK folks too, maybe you are interested in it too. > (The patch is based on your great DEBUG_FISSION option) > > The question is if we should make using ld.gold default or not. > In this case I meant it can be disabled manually or the config > could disable it automatically if it isn't available. (with or > without warning the developer) > > Or do you think if using gold linker should be disabled by default > for some reason, and should be an opt-in option? We are using gold on the GTK buildbots, but we resort to the path trick: export PATH="/usr/lib/gold-ld:${PATH}" I never heard before of this "-fuse-ld=gold" parameter. Is that option supported upstream? because after a bit of googling it seems to be a debian/ubuntu only thing. So perhaps that can break non debian-based (fedora/arch/suse/tizen..). Check: https://bugs.webkit.org/show_bug.cgi?id=89312 (In reply to comment #4) > I never heard before of this "-fuse-ld=gold" parameter. It seems to be a normal GCC option: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html I don't know when it was introduced, though. Probably not long ago. I don't have anything against using gold by default if available as long as it doesn't break the build with one of our supported versions of gcc. Then again it's also something that users can set up themselves easily in their build environments (in fact that's what I do), so ... I checked, it is available in upstream GCC too from 4.8.0: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55470 WebKit requires at least 4.7: http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h?order=name#L68 But it isn't a problem, because the cmake script can check if fuse-ls=gold GCC option is available or not. If no, we simply don't add it to C(XX)FLAGS. Created attachment 241101 [details]
Patch
I think we should use ld.gold if it is available, and disable it only if it isn't available.
Attachment 241101 [details] did not pass style-queue:
ERROR: Source/cmake/OptionsCommon.cmake:52: One space between command "else" and its parentheses, should be "else (" [whitespace/parentheses] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 241102 [details]
Patch
style fixed
any opinion? pros/cons? ping? Hey, I don't really have a strong opinion about the idea of using gold by default. As I said earlier, choosing which linker to use is already easy enough My question is: how would you disable it with your patch? Is there an easy way to use the GNU linker even if gold is available? (In reply to comment #12) > Hey, I don't really have a strong opinion about the idea of using gold > by default. As I said earlier, choosing which linker to use is already > easy enough Yes, it is easy, you have to copy ld.gold somewhere else with ld name and put it in the path before the normal ld. Why not use it by default everywhere without local hacking? Now ld.bfd is the default and I would like to change it to ld.gold. > My question is: how would you disable it with your patch? Is there an > easy way to use the GNU linker even if gold is available? Good point, it isn't trivial with the attached patch. I'll update it immediately to let folks use the good old linker too. Created attachment 241944 [details]
Patch
Now ld.gold is used if it is available and isn't disabled explicitly with USE_LD_GOLD=OFF option. Additionally I removed the redundant check from DEBUG_FISSION code path.
(In reply to comment #13) > > choosing which linker to use is already easy enough > Yes, it is easy, you have to copy ld.gold somewhere else with ld > name and put it in the path before the normal ld. You don't really need to copy anything: $ PATH=/usr/lib/gold-ld:$PATH Tools/Scripts/build-webkit ... > Good point, it isn't trivial with the attached patch. I'll update it > immediately to let folks use the good old linker too. This patch looks good to me, thanks! (In reply to comment #15) > You don't really need to copy anything: > $ PATH=/usr/lib/gold-ld:$PATH Tools/Scripts/build-webkit ... Good point, I didn't realize that gold-ld directory is installed by default. :) Comment on attachment 241944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241944&action=review > ChangeLog:3 > + [EFL] Use ld.gold if it is available to speedup builds Please, remove the EFL prefix from the ChangeLog and commit message before landing this. Committed r176442: <http://trac.webkit.org/changeset/176442> |