Bug 187108 – Add configuration for automatic process pre-warming

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ben Richards]

We should unify the behavior of warmInitialProcess() and didReachGoodTimeToPrewarm() so that we end up with one true way to have a pre-warmed process to use (a) on launch (b) on navigation (c) on new tab (d) on new window (e) maybe some other case I’m forgetting

[Ben Richards]

Created attachment 343876 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 343967 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 344157 [details]
Patch

[Chris Dumez]

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

[Ben Richards]

Created attachment 344216 [details]
Patch

[Sam Weinig]

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.

[Sam Weinig]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 345050 [details]
Patch

[Brady Eidson]

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.

[Ryosuke Niwa]

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.

[Ben Richards]

Created attachment 345386 [details]
Patch

[Ben Richards]

Created attachment 345397 [details]
Patch

[Ben Richards]

Created attachment 346231 [details]
Patch

[Ben Richards]

Created attachment 346232 [details]
Patch

[Ryosuke Niwa]

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.

[Ben Richards]

Created attachment 346239 [details]
Patch for landing

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

I'm gonna just fix these two nits and land it myself.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r234445: <https://trac.webkit.org/changeset/234445>

[Ryosuke Niwa]

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

[Ben Richards]

Thanks!

[WebKit Commit Bot]

Re-opened since this is blocked by bug 188224

[Truitt Savell]

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

[Radar WebKit Bug Importer]

<rdar://problem/42840504>

[Ben Richards]

Created attachment 346447 [details]
Patch

[Ryosuke Niwa]

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.

[Ben Richards]

Created attachment 346448 [details]
Patch for landing

[WebKit Commit Bot]

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

[Ryosuke Niwa]

Hm... this patch doesn't seem to build on trunk :(

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ben Richards]

Created attachment 346459 [details]
Patch for landing

[Ben Richards]

Should be good now

[Ryosuke Niwa]

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.

[Ben Richards]

Created attachment 346493 [details]
Patch for landing

[Ben Richards]

Dang it, sorry one more try

[Ben Richards]

Created attachment 346496 [details]
Patch for landing

[Ben Richards]

Created attachment 346497 [details]
Patch for landing

[Ben Richards]

Fixed ChangeLog, this one should be ready to go.

[WebKit Commit Bot]

Comment on attachment 346497 [details]
Patch for landing

Clearing flags on attachment: 346497

Committed r234560: <https://trac.webkit.org/changeset/234560>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.