Bug 190758 - Re-enable LTO support
Summary: Re-enable LTO support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-19 12:26 PDT by Keith Rollin
Modified: 2019-03-22 11:56 PDT (History)
16 users (show)

See Also:


Attachments
Patch (24.25 KB, patch)
2019-03-18 15:41 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later. (35.81 KB, patch)
2019-03-20 17:54 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.63 MB, application/zip)
2019-03-21 00:11 PDT, Build Bot
no flags Details
Patch (35.83 KB, patch)
2019-03-22 11:29 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2018-10-19 12:26:32 PDT
Bug 187785 (r235381) added support for building WebKit with LTO. An infrequent crashing bug in libLTO.dylib required us to back this out in r235412. When a fixed libLTO.dylib is available, re-enable WebKit's support for building with LTO.
Comment 1 Radar WebKit Bug Importer 2018-10-19 12:26:47 PDT
<rdar://problem/45413233>
Comment 2 Alexey Proskuryakov 2018-10-19 12:37:08 PDT
Do we have a consensus that other consequences (like build time impact) are acceptable to the project?
Comment 3 Keith Rollin 2018-10-19 13:22:39 PDT
LTO is being enabled by default only for Production builds. So consensus should be amount the parties that perform those builds, which I guess is your group and Apple's central build center.

Consideration should be based on the actual amount of time added for the build machines used (e.g., an LTO-thin build takes about 2-3 minutes longer on my development system than a non-LTO build (18m27s vs. 16m5s)) as well as the direct (faster, smaller binaries) and indirect (e.g., support for CFI (https://clang.llvm.org/docs/ControlFlowIntegrity.html) and other security measures) benefits of LTO.

Hopefully, the increased build time has been mitigated over the last year through the use of unified-source builds, something which has still to be completed, and will further be mitigated with the enabling of XCBuild.
Comment 4 Alexey Proskuryakov 2018-10-25 10:19:04 PDT
IIRC another objection was that people working on performance weren't OK with having different performance characteristics in release and production builds.
Comment 5 Keith Rollin 2019-03-18 15:41:46 PDT
Created attachment 365078 [details]
Patch
Comment 6 Keith Rollin 2019-03-18 15:45:18 PDT
Note, there are 32-bit artifacts in this patch. As I understand it, those can be removed. I'd like to do that in a follow-up patch. Removing that support will require extensive retesting, which would affect our being able to take advantage of the LTO benefits now.
Comment 7 Build Bot 2019-03-18 15:45:48 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 8 Alexey Proskuryakov 2019-03-18 16:02:23 PDT
Comment on attachment 365078 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Enable ThinLTO support in Production builds

Need build performance measurement before I can support this change. Continued an e-mail thread.
Comment 9 Keith Rollin 2019-03-19 00:55:18 PDT
The iOS build is failing.

For ThinLTO:

Ld /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit normal arm64e
    cd /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/Sources/WebKit2
    export IPHONEOS_DEPLOYMENT_TARGET=12.2
    export PATH="/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/usr/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
    /Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Toolchains/iOS12.2.xctoolchain/usr/bin/clang++ -arch arm64e -dynamiclib -isysroot /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk -L/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/Symbols/BuiltProducts -L/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/Shared/Symbols/ios -L/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks/WebCore.framework/Frameworks -F/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/Symbols/BuiltProducts -F/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/Shared/Symbols/ios -F/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -iframework /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -iframework /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/Frameworks -iframework /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -filelist /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit.LinkFileList -Xlinker -reexport_framework -Xlinker WebKitLegacy -install_name /System/Library/Frameworks/WebKit.framework/WebKit -miphoneos-version-min=12.2 -dead_strip -Xlinker -object_path_lto -Xlinker /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit_lto.o -Xlinker -final_output -Xlinker /System/Library/Frameworks/WebKit.framework/WebKit -stdlib=libc++ -flto=thin -Xlinker -cache_path_lto -Xlinker /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/LTOCache -Wl,-order_file,/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/AppleInternal/OrderFiles/WebKit.order -iframework/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -Wl,-unexported_symbol -Wl,__ZTISt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTISt9exception -Wl,-unexported_symbol -Wl,__ZTSSt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTSSt9exception -Wl,-unexported_symbol -Wl,__ZdlPvS_ -Wl,-unexported_symbol -Wl,__ZnwmPv -Wl,-unexported_symbol -Wl,__Znwm -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC2EOS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC1EOS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEaSEDn -Wl,-unexported_symbol -Wl,__ZNKSt3__18functionIFvN7WebCore12PolicyActionEEEclES2_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEE4swapERS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC1ERKS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC2ERKS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEED1Ev -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEED2Ev -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEaSERKS4_ -Wl,-unexported_symbol -Wl,__ZTVNSt3__117bad_function_callE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_14basic_iostreamIcS2_EE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE16_NS_13basic_ostreamIcS2_EE -Wl,-unexported_symbol -Wl,__ZTTNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE -Wl,-unexported_symbol -Wl,__ZTVNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEE -Wl,-unexported_symbol -Wl,__ZTVNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE8_NS_13basic_ostreamIcS2_EE -lobjc -framework CFNetwork -framework CoreAudio -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework IOKit -framework WebKitLegacy -lnetwork -lAccessibility -framework AssertionServices -framework CorePDF -framework CorePrediction -framework DeviceIdentity -framework GraphicsServices -framework IOSurface -weak-lwebrtc -framework MobileCoreServices -lMobileGestalt -framework OpenGLES -framework PDFKit -framework SafariSafeBrowsing -framework UIKit -framework URLFormatting -framework JavaScriptCore -licucore -framework QuartzCore -framework Security -framework WebCore -compatibility_version 1 -current_version 608.1.10 -Xlinker -dependency_info -Xlinker /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit_dependency_info.dat -o /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit
invalid llvm.ptrauth global: global doesn't have an initializer
{ i8*, i32, i64, i64 }* @_ZN3WTF8in_placeILm0EEENS_12in_place_tagERNS_18__in_place_private14__value_holderIXT_EEE.ptrauth.llvm.6030729957940808222
LLVM ERROR: Broken module found, compilation aborted!
clang: error: linker command failed with exit code 1 (use -v to see invocation)


For Full LTO, it gets past that but then fails at:

GenerateDSYMFile /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Symbols/WebCore.framework.dSYM /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Root/System/Library/PrivateFrameworks/WebCore.framework/WebCore
    cd /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/Sources/WebCore
    export PATH="/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/usr/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
    /Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Toolchains/iOS12.2.xctoolchain/usr/bin/dsymutil /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Root/System/Library/PrivateFrameworks/WebCore.framework/WebCore -o /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Symbols/WebCore.framework.dSYM
Command /Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Toolchains/iOS12.2.xctoolchain/usr/bin/dsymutil failed with exit code 10
Comment 10 Keith Rollin 2019-03-19 02:45:32 PDT
It's failing for arm64e, but not arm64.
Comment 11 Keith Rollin 2019-03-20 17:54:25 PDT
Created attachment 365450 [details]
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.
Comment 12 Build Bot 2019-03-21 00:11:13 PDT
Comment on attachment 365450 [details]
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.

Attachment 365450 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11593723

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 13 Build Bot 2019-03-21 00:11:15 PDT
Created attachment 365508 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 14 Keith Rollin 2019-03-21 10:56:10 PDT
The bot-reported error is because the following is failing to load the AVFoundation:

SOFT_LINK_CONSTANT(AVFoundation, AVEncoderBitRateKey, NSString *)

Testing locally, both with and without LTO, I get the following error for http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html:

+ CONSOLE MESSAGE: line 2659: Error: assert_approx_equals: Red channel of the pixel at (25, 25) expected 255 +/- 20 but got 0
  
  
- PASS MediaRecorder can successfully record the video for a audio-video stream 
+ Harness Error (FAIL), message = Error: assert_approx_equals: Red channel of the pixel at (25, 25) expected 255 +/- 20 but got 0
  
+ TIMEOUT MediaRecorder can successfully record the video for a audio-video stream Test timed out
+
Comment 15 Keith Rollin 2019-03-21 12:12:37 PDT
Weird. Sometimes that test gives this slightly different error:

+ CONSOLE MESSAGE: line 2659: Error: assert_approx_equals: Red channel of the pixel at (20, 20) expected 0 +/- 20 but got 254


- PASS MediaRecorder can successfully record the video for a audio-video stream 
+ Harness Error (FAIL), message = Error: assert_approx_equals: Red channel of the pixel at (20, 20) expected 0 +/- 20 but got 254

+ TIMEOUT MediaRecorder can successfully record the video for a audio-video stream Test timed out
+
Comment 16 Keith Rollin 2019-03-21 13:39:02 PDT
There should be no way that the LTO changes affected the AVEncoderBitRateKey error. LTO should be disabled for high Sierra because it comes with Xcode 9.3. LTO is enabled only with Xcode 10.2 or later.
Comment 17 Keith Rollin 2019-03-21 13:40:27 PDT
I've checked that, according to the SDK, AVEncoderBitRateKey exists in AVFoundation in 10.13.
Comment 18 Keith Rollin 2019-03-21 13:50:00 PDT
AVFoundation exists on 10.13. AVEncoderBitRateKey exists within it. Though that second part should not be relevant, since the error is with loading the library, not any symbol within the library.
Comment 19 Keith Rollin 2019-03-21 14:17:08 PDT
Eric Carlson tells me that the issue with AVFoundation is a pre-existing known issue: <rdar://problem/47483831>. So the posted patch should be vindicated and OK to review and land.
Comment 20 Keith Rollin 2019-03-21 14:43:01 PDT
Per the email thread Alexey mentioned, the Production build time hit is acceptable.
Comment 21 Daniel Bates 2019-03-21 20:36:40 PDT
Comment on attachment 365450 [details]
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.

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

Ok as-s. No need to address comments, though addressing them would make this patch better in my opinion. cq- for you to contemplate. My comments apply to each file, including ChangeLogs in this patch..

> Source/ThirdParty/ANGLE/ChangeLog:24
> +        - If building with `build-root`, specify --lto={non,thin,full} on the

non

> Source/JavaScriptCore/Configurations/Base.xcconfig:182
> +// Disable on all platforms other than macos, due to rdar://problem/49013399

macos

missing period at the end. <> around URL

> Source/JavaScriptCore/Configurations/Base.xcconfig:192
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1010 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1000 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000 = $(WK_XCODE_VERSION_BEFORE_10_2_1000_$(XCODE_VERSION_MINOR));

Ok as-is. Could be ordered based on increasing major version.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:84
> +// Disable on all platforms other than macos, due to rdar://problem/49013399

See comments above.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:93
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1010 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1000 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000 = $(WK_XCODE_VERSION_BEFORE_10_2_1000_$(XCODE_VERSION_MINOR));

Ok as-is. Same ordering suggestion as above.
Comment 22 Keith Rollin 2019-03-22 11:29:46 PDT
Created attachment 365750 [details]
Patch
Comment 23 WebKit Commit Bot 2019-03-22 11:56:47 PDT
Comment on attachment 365750 [details]
Patch

Clearing flags on attachment: 365750

Committed r243396: <https://trac.webkit.org/changeset/243396>
Comment 24 WebKit Commit Bot 2019-03-22 11:56:49 PDT
All reviewed patches have been landed.  Closing bug.