| Summary: | Build system support for LTO | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Rollin <krollin> | ||||||||||||||
| Component: | Tools / Tests | Assignee: | Keith Rollin <krollin> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | achristensen, ap, bfulgham, commit-queue, dbates, ddkilzer, dino, eric.carlson, ews-watchlist, graouts, keith_miller, kondapallykalyan, lforschler, mark.lam, mitz, msaboff, saam, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 345291 [details]
Patch
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE Comment on attachment 345291 [details] Patch Attachment 345291 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8580726 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html Created attachment 345309 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 345367 [details]
Patch
Created attachment 345371 [details]
Rebase, and wipe-out the previous patch, which was uploaded to this bug by mistake
Comment on attachment 345371 [details] Rebase, and wipe-out the previous patch, which was uploaded to this bug by mistake View in context: https://bugs.webkit.org/attachment.cgi?id=345371&action=review > Source/ThirdParty/ANGLE/ChangeLog:22 > + - To create an Engineering build with LTO using Xcode, temporarily > + modify Internal/Configurations/HaveInternalSDK.xcconfig to include > + "LTO_MODE=thin" or "LTO_MODE=full". This is not good advice (and also, this advice only applies to WebKit contributors from Apple, so I’m not sure it belongs in this ChangeLog file), because making “temporary” changes to files that are under source control is cumbersome and error-prone. A better mechanism (which is already used in some of the Apple-internal projects in the Safari stack) is for DebugRelease.xcconfig to do something like #include? "../../../LocalOverrides.xcconfig" (with the ../s leading all the way up to the top level of the repo), and have LocalOverrides.xcconfig added to the top-level .gitignore file and svn:ignore property. Then the advice here changes to “to create and Engineering builds with LTO using Xcode, temporarily change the top-level LocalOverrides.xcconfig to include "LTO_MODE=thin" or "LTO_MODE=full"”. > Source/ThirdParty/ANGLE/ChangeLog:33 > + - To create a Production build with LTO, edit each lto.xcconfig file, > + changing the definition of WK_DEFAULT_LTO_MODE_Production from > + $(WK_LTO_NONE) to $(WK_LTO_THIN) or $(WK_LTO_FULL). To automate > + this, you might `cd` to WebKit's "Sources" directory and the run the > + following script: > + > + for XCCONFIG_FILE in "$(find . -name lto.xcconfig)" > + do > + sed -i '' -Ee 's/WK_DEFAULT_LTO_MODE_Production = \$\(.*\)/WK_DEFAULT_LTO_MODE_Production = \$(WK_LTO_FULL)/' "${XCCONFIG_FILE}" > + done This advice also seems like it doesn’t belong in this ChangeLog file, because it’s impractical for contributors from outside Apple to build using the Production configuration, and because the Production configuration only supports the install action, which is only available using the xcodebuild command, and when using that command, build settings can be passed on the command line. Apple contributors use an Apple-internal script to drive the production build, and that script should probably be doing this when it invokes (the thing that invokes) xcodebuild. We can discuss this in more detail separately from this review, but for now I think we can do with just removing this piece of advice from the ChangeLog. > Source/ThirdParty/ANGLE/ChangeLog:47 > + Thin: 14.0 min 17.0 min xx.x min 15.5 min > + Full: 25.0 min 27.0 min xx.x min 26.7 min Does xx.x here indicate that by the time you wrote this it hasn’t finished yet? :) > Source/ThirdParty/ANGLE/ChangeLog:50 > + * Configurations/lto.xcconfig: Added. Two departures from our usual style: we tend to capitalize acronyms (so: LTO.xcconfig), and we have avoided splitting factoring things into their own separate .xcconfig files if those just end up being #included by one file. Perhaps you did this splitting-out in order to support the above “edit all .xcconfig files” workflows. I think there are better alternatives to those workflows, and therefore I think the lto.xcconfig files should be folded back into the Base.xcconfig files. > Source/ThirdParty/ANGLE/Configurations/lto.xcconfig:29 > +// if they set the LTO_MODE environment variable to "full", "thin", or "none". s/environment variable/build setting/ In recent years, we have been trying to prefix all custom build settings with WK_ (with the exceptions of our ENABLE_… build settings and build settings whose names are just standard build settings with custom suffixes). We do this because otherwise the lack of name spacing makes it difficult to tell, when looking at a build setting, whether it has special meaning to the build system (either as something it provides or something it consumes) or not; and to avoid clashing with future (or even present, undocumented) built-in settings. Specifically here, I can’t tell whether LTO_MODE means something to Xcode, or only LLVM_LTO does. > Source/ThirdParty/ANGLE/Configurations/lto.xcconfig:33 > +LLVM_LTO = $(WK_LLVM_LTO_$(WK_USER_LTO_MODE)); Our normal style is to put one space on either side of the = sign. > Source/ThirdParty/ANGLE/Configurations/lto.xcconfig:53 > +WK_DEFAULT_LTO_MODE_Production = $(WK_LTO_NONE); > +WK_DEFAULT_LTO_MODE_Release = $(WK_LTO_NONE); > +WK_DEFAULT_LTO_MODE_Debug = $(WK_LTO_NONE); Obviously right now the per-configuration defaults are unnecessary, since they’re all the same. And anything having to do with the Debug and Release configurations is better dealt with in DebugRelease.xcconfig. > Source/ThirdParty/ANGLE/Configurations/lto.xcconfig:66 > +WK_LTO_FULL = $(WK_LTO_FULL_$(WK_XCODE_SUPPORTS_LTO)); > +WK_LTO_FULL_YES = full; > +WK_LTO_FULL_NO = none; > + > +WK_LTO_THIN = $(WK_LTO_THIN_$(WK_XCODE_SUPPORTS_LTO)); > +WK_LTO_THIN_YES = thin; > +WK_LTO_THIN_NO = none; > + > +WK_LTO_NONE = none; It seems strange and less readable to me that we’re doing the Xcode-supports-LTO-conditionals here rather than right at the top. Why not just do this at the top: LLVM_LTO = $(LLVM_LTO_$(WK_XCODE_SUPPORTS_LTO)); LLVM_LTO_NO = none; LLVM_LTO_YES = $(WK_LLVM_LTO_$(WK_USER_LTO_MODE)); then WK_LTO_FULL, WK_LTO_THIN, and WK_LTO_NONE become unnecessary because they’re constants, and maybe you can further simplify. > Source/ThirdParty/ANGLE/Configurations/lto.xcconfig:73 > +WK_XCODE_VERSION_0700 = NO; > +WK_XCODE_VERSION_0800 = NO; You can’t just take a generic name like “WK_XCODE_VERSION” for this! This says whether Xcode is version 9 or later. Or, to reverse the meaning and use our more conventional naming from WebKitTargetConditionals.xcconfig, whether the Xcode version is before 9: WK_XCODE_SUPPORTS_LTO = $(WK_NOT_$(WK_XCODE_VERSION_BEFORE_9_$(XCODE_VERSION_MAJOR))); WK_XCODE_VERSION_BEFORE_9_0700 = YES; WK_XCODE_VERSION_BEFORE_9_0800 = YES; Comment on attachment 345371 [details] Rebase, and wipe-out the previous patch, which was uploaded to this bug by mistake Attachment 345371 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8596260 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 345432 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to mitz from comment #8) > Comment on attachment 345371 [details] > A better mechanism (which is already used in some of the Apple-internal > projects in the Safari stack) is for DebugRelease.xcconfig to do something > like > #include? "../../../LocalOverrides.xcconfig" > (with the ../s leading all the way up to the top level of the repo), and > have LocalOverrides.xcconfig added to the top-level .gitignore file and > svn:ignore property. Done. I'm not sure I can affect the svn:ignore property via the commit-bot, though, and am pretty sure I can't do it from my git repository, so I may have to follow-up on my next patch to set svn:ignore from an svn check-out. > We can discuss this in more detail separately from this review, but for now > I think we can do with just removing this piece of advice from the ChangeLog. Done. > > Source/ThirdParty/ANGLE/ChangeLog:50 > > + * Configurations/lto.xcconfig: Added. > > Two departures from our usual style: we tend to capitalize acronyms (so: > LTO.xcconfig), and we have avoided splitting factoring things into their own > separate .xcconfig files if those just end up being #included by one file. > Perhaps you did this splitting-out in order to support the above “edit all > .xcconfig files” workflows. Actually, there used to be just one lto.xcconfig file that all Base.xcconfig files included. It was only later that I found out that this wouldn't work for Production builds because that one shared lto.xcconfig file wouldn't be available in the Production build environment (my understanding is that each WebKit project is built individually and in isolation from the other projects, such that there is no one shared location in which to place the lto.xcconfig file). In response, I simply duplicated the lto.xcconfig file across all projects. So that's how I got into this situation. > I think there are better alternatives to those > workflows, and therefore I think the lto.xcconfig files should be folded > back into the Base.xcconfig files. Done. > In recent years, we have been trying to prefix all custom build settings > with WK_ (with the exceptions of our ENABLE_… build settings and build > settings whose names are just standard build settings with custom suffixes). Done. > Our normal style is to put one space on either side of the = sign. Done. > Obviously right now the per-configuration defaults are unnecessary, since > they’re all the same. And anything having to do with the Debug and Release > configurations is better dealt with in DebugRelease.xcconfig. Done. > It seems strange and less readable to me that we’re doing the > Xcode-supports-LTO-conditionals here rather than right at the top. Why not > just do this at the top: I think I've simplified along the lines you suggest. The result is certainly smaller and more understandable. Created attachment 347948 [details]
Patch
Comment on attachment 347948 [details] Patch Clearing flags on attachment: 347948 Committed r235381: <https://trac.webkit.org/changeset/235381> All reviewed patches have been landed. Closing bug. |
Add support for building WebKit with LTO support (both "full" and "thin") on macOS and iOS. By default, LTO is not enabled. You need to enable it for the build mechanism you are using. - To create an Engineering (Release or Debug) build with LTO using `make` from the command line, include LTO_MODE=thin or LTO_MODE=full. For example, `make LTO_MODE=thin release`. - To create an Engineering build with LTO using `build-webkit`, include --lto-mode=thin or --lto-mode=full on the command line. - To create an Engineering build with LTO using Xcode, temporarily modify Internal/Configurations/HaveInternalSDK.xcconfig to include "LTO_MODE=thin" or "LTO_MODE=full". - To create a Production build with LTO, edit each lto.xcconfig file, changing the definition of WK_DEFAULT_LTO_MODE_Production from $(WK_LTO_NONE) to $(WK_LTO_THIN) or $(WK_LTO_FULL). To automate this, you might `cd` to WebKit's "Sources" directory and the run the following script: for XCCONFIG_FILE in "$(find . -name lto.xcconfig)" do sed -i '' -Ee 's/WK_DEFAULT_LTO_MODE_Production = \$\(.*\)/WK_DEFAULT_LTO_MODE_Production = \$(WK_LTO_FULL)/' "${XCCONFIG_FILE}" done Ultimately, the idea is to enable LTO=full for Production builds so that Safari and the WebKit frameworks benefit from it, but to leave it off for Engineering builds due to the increased build times. For comparison, here are the results of creating a Release build of WebKit + Safari on a fully loaded 2017 iMac Pro: Mac iOS ------------------ ------------------ Xcode make Xcode make -------- -------- -------- -------- None: 11.5 min 14.5 min 9.9 min 12.8 min Thin: 14.0 min 17.0 min xx.x min 15.5 min Full: 25.0 min 27.0 min xx.x min 26.7 min