Summary: | Add configuration for automatic process pre-warming | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ben Richards <benton_richards> | ||||||||||||||||||||||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Ben Richards <benton_richards> | ||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | beidson, benton_richards, cdumez, commit-queue, ews-watchlist, lforschler, mitz, rniwa, sam, tsavell | ||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 188224 | ||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Ben Richards
2018-06-27 11:26:18 PDT
Created attachment 343876 [details]
Patch
Comment on attachment 343876 [details] Patch Attachment 343876 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8376846 Number of test failures exceeded the failure limit. Created attachment 343883 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343876 [details] Patch Attachment 343876 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8376731 Number of test failures exceeded the failure limit. Created attachment 343884 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 343876 [details] Patch Attachment 343876 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8378278 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 343893 [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 343967 [details]
Patch
Comment on attachment 343967 [details] Patch Attachment 343967 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8392553 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html Created attachment 343999 [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
Created attachment 344157 [details]
Patch
Comment on attachment 344157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344157&action=review > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:147 > + bool processPrewarmingEnabled() const { return m_processPrewarmingEnabled; } Boolean variables need a prefix as per webkit coding style. Should probably be 'isProcessPrewarmingEnabled() / m_isProcessPrewarmingEnabled'. > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:191 > + bool m_processSwapsOnNavigation { true }; I do not think we should change the default value here. > Source/WebKit/UIProcess/WebProcessPool.cpp:306 > + // Warm initial process WebKit comments need to end with a period. > Source/WebKit/UIProcess/WebProcessPool.cpp:307 > + if (m_configuration->processPrewarmingEnabled()) Not really needed since you also check this inside didReachGoodTimeToPrewarm(). Created attachment 344216 [details]
Patch
Comment on attachment 344216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344216&action=review > Source/WebCore/page/Settings.yaml:758 > +webProcessPrewarmingEnabled: > + initial: true Traditionally WebCore does not know about the WebProcess, which is a WebKit concept. Any reason this is absolutely needed? > Source/WebKit/Shared/WebPreferences.yaml:1280 > +WebProcessPrewarmingEnabled: > + type: bool > + defaultValue: true I see the APIProcessPoolConfiguration option being checked, but I am not clear on how this preference is used. Comment on attachment 344216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344216&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1301 > + if (!m_configuration->processSwapsOnNavigation() || !m_configuration->isProcessPrewarmingEnabled()) > return; Why is this only done when processSwapsOnNavigation is enabled? Comment on attachment 344216 [details] Patch Attachment 344216 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8432058 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html Created attachment 344259 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 345050 [details]
Patch
Comment on attachment 345050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345050&action=review In general, I do not see the value of making this an entirely different experimental feature from process swapping. We're not interesting in pre-warming more than one process with current WebKit behavior, and we also know that we always want prewarming *with* process swapping. Let's dump all of the configuration related to enabling/disabling this separately. > Source/WebKit/UIProcess/API/C/WKContext.cpp:508 > -void WKContextWarmInitialProcess(WKContextRef contextRef) > +void WKContextWarmNewProcess(WKContextRef contextRef) > { > - toImpl(contextRef)->warmInitialProcess(); > + toImpl(contextRef)->warmNewProcess(); This change will break Safari. > Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:395 > -- (void)_warmInitialProcess > +- (void)_warmNewProcess > { > - _processPool->warmInitialProcess(); > + _processPool->warmNewProcess(); This may-or-may-not break Safari. I don't think we should change it. > Source/WebKit/UIProcess/WebProcessPool.h:159 > + void setMaximumNumberOfPrewarmedProcesses(unsigned); AFAICT nobody actually calls this method. Comment on attachment 345050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345050&action=review > Source/WebKit/Shared/WebPreferences.yaml:1299 > + humanReadableDescription: "Pre-warm WebContent processes for swaps" I don't think we need to say "for swaps" since we're also using it when we open a new tab. > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:58 > + Nit: Whitespace. > Source/WebKit/UIProcess/API/C/WKContext.cpp:506 > -void WKContextWarmInitialProcess(WKContextRef contextRef) > +void WKContextWarmNewProcess(WKContextRef contextRef) This will break Safari builds. Why don't we add an empty implementation so that we don't break the build of Safari until the call gets removed. > Source/WebKit/UIProcess/WebPageProxy.cpp:3763 > + > + notifyProcessPoolToPrewarm(); This deserves a change log comment clarifying why we're moving this function call. > Source/WebKit/UIProcess/WebProcessPool.cpp:144 > +bool WebProcessPool::isProcessPrewarmingEnabled { false }; I think should just be a static local variable with s_isProcessPrewarmingEnabled. Created attachment 345386 [details]
Patch
Created attachment 345397 [details]
Patch
Created attachment 346231 [details]
Patch
Created attachment 346232 [details]
Patch
Comment on attachment 346231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346231&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:407 > - if (!m_processes.isEmpty()) > + if (m_processes.size() != m_prewarmedProcessCount) > CRASH(); We should add mention in the change log that this is needed to preserve the existing behavior that some WebKit embedders are prewarming processes before setting maximum number of processes. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:54 > + EXPECT_EQ(2u, [processPool _prewarmedWebProcessCount]); Set the number of maximum processes here to make sure we don't crash. Created attachment 346239 [details]
Patch for landing
Comment on attachment 346239 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=346239&action=review > Source/WebKit/ChangeLog:25 > + (WebKit::WebProcessPool::setMaximumNumberOfProcesses): Condition changed so that warmInitialProcess doesn't result in a crash if called before this function. warmInitialProcess itself doesn't crash but calling setMaximumNumberOfProcesses would. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. This should be 2018. I'm gonna just fix these two nits and land it myself. Committed r234443: <https://trac.webkit.org/changeset/234443> Comment on attachment 346239 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=346239&action=review > Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:38 > +@property (nonatomic) NSInteger maximumPrewarmedProcessCount; This declaration is missing availability information. Committed r234445: <https://trac.webkit.org/changeset/234445> (In reply to mitz from comment #30) > Comment on attachment 346239 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346239&action=review > > > Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:38 > > +@property (nonatomic) NSInteger maximumPrewarmedProcessCount; > > This declaration is missing availability information. Oops, fixed it in https://trac.webkit.org/changeset/234445/webkit. Thanks! Re-opened since this is blocked by bug 188224 After https://trac.webkit.org/changeset/234443/webkit We have 3 api failures across all debug platforms and 2 across all release platforms: https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/builds/8886/steps/run-api-tests/logs/stdio Build page: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20%28Tests%29/builds/4965 Rolled out in https://trac.webkit.org/changeset/234459/webkit Created attachment 346447 [details]
Patch
Comment on attachment 346447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346447&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1336 > + Nit: Whitespace here. > Tools/TestWebKitAPI/Tests/WebKitCocoa/InitialWarmedProcessUsed.mm:43 > + Nit: Whitespace. > Tools/TestWebKitAPI/Tests/WebKitCocoa/InitialWarmedProcessUsed.mm:46 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/InitialWarmedProcessUsed.mm:50 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/InitialWarmedProcessUsed.mm:52 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/InitialWarmedProcessUsed.mm:58 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:41 > + Nit: Whitespace. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:43 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:45 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:47 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:49 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:51 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:53 > + Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/SetMaximumPrewarmedProcessCount.mm:55 > + Ditto. > ChangeLog:8 > + * WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme: Let's revert this change. Created attachment 346448 [details]
Patch for landing
Comment on attachment 346448 [details] Patch for landing Rejecting attachment 346448 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: R_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SUBTLE_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DENABLE_MANUAL_SANDBOXING -DHAVE_CORE_PREDICTION -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -DFRAMEWORK_NAME=WebKit -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -fstrict-aliasing -Wprotocol -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.12 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/ForwardingHeaders -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat-security -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Carbon.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Quartz.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebKit2Prefix-arfuituxtttvsjdvrfdbfbwszmrc/WebKit2Prefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/APIDataCocoa.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/APIDataCocoa.dia -c /Volumes/Data/EWS/WebKit/Source/WebKit/Shared/Cocoa/APIDataCocoa.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/APIDataCocoa.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/_WKProcessPoolConfiguration.o UIProcess/API/Cocoa/_WKProcessPoolConfiguration.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: https://webkit-queues.webkit.org/results/8745683 Hm... this patch doesn't seem to build on trunk :( Comment on attachment 346448 [details] Patch for landing Attachment 346448 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8745973 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html Created attachment 346457 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 346459 [details]
Patch for landing
Should be good now Comment on attachment 346459 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=346459&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1014 > + Nit: Whitespace. > WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:3 > - version = "1.7"> > + version = "1.3"> Gotta revert this. Created attachment 346493 [details]
Patch for landing
Dang it, sorry one more try Created attachment 346496 [details]
Patch for landing
Created attachment 346497 [details]
Patch for landing
Fixed ChangeLog, this one should be ready to go. Comment on attachment 346497 [details] Patch for landing Clearing flags on attachment: 346497 Committed r234560: <https://trac.webkit.org/changeset/234560> All reviewed patches have been landed. Closing bug. |