Skip to content

Improve launch time of WebKit processes, v2#18651

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
pvollan:eng/Improve-launch-time-of-WebKit-processes-v2
Oct 6, 2023
Merged

Improve launch time of WebKit processes, v2#18651
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
pvollan:eng/Improve-launch-time-of-WebKit-processes-v2

Conversation

@pvollan
Copy link
Contributor

@pvollan pvollan commented Oct 4, 2023

9590b61

Improve launch time of WebKit processes, v2
https://bugs.webkit.org/show_bug.cgi?id=261966
rdar://problem/115906222

Reviewed by Brent Fulgham.

To benefit of the launch time improvements by launching WebKit processes as extensions, this patch implements
this approach by using ExtensionKit for launching. The feature is currently behind a flags, which is not
being turned on in this patch. The extensions are currently being installed in the WebKit framework.

* Source/WebKit/Configurations/BaseExtension.xcconfig:
* Source/WebKit/Platform/spi/Cocoa/ExtensionKitSPI.h: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h.
* Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:
* Source/WebKit/Scripts/process-entitlements.sh:
* Source/WebKit/Shared/API/Cocoa/WKMain.h:
* Source/WebKit/Shared/API/Cocoa/WKMain.mm:
(WKExtensionEventHandler):
(WKExtensionMain): Deleted.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.entitlements: Removed.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift:
(GPUProcessExtension.handle(_:)):
(AuxiliaryProcessExtension.main): Deleted.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h:
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.mm:
(handleNewConnection):
(extensionMain): Deleted.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/GPUProcessExtension.swift: Renamed from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift.
(GPUProcessExtension.handle(_:)):
* Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift.
(NetworkingProcessExtension.handle(_:)):
* Source/WebKit/Shared/AuxiliaryProcessExtensions/WebContentProcessExtension.swift: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift.
(WebContentProcessExtension.handle(_:)):
* Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:
* Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:
(WebKit::XPCServiceEventHandler):
(WebKit::XPCServiceMain):
* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
* Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:
(WebKit::serviceNameAndIdentifier):
(WebKit::launchWithExtensionKit):
(WebKit::ProcessLauncher::launchProcess):
(WebKit::ProcessLauncher::finishLaunchingProcess):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/268990@main

7755c87

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 webkitpy ✅ 🧪 ios-wk2-wpt ⏳ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ⏳ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@pvollan pvollan self-assigned this Oct 4, 2023
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Oct 4, 2023
@pvollan pvollan requested a review from achristensen07 October 4, 2023 22:31
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 4, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 4, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from 15a8da9 to e2e352a Compare October 4, 2023 23:01
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 4, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from e2e352a to 7d2875e Compare October 5, 2023 02:59
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from 7d2875e to b64dc60 Compare October 5, 2023 04:00
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Oct 5, 2023

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from b64dc60 to ab156ae Compare October 5, 2023 14:54
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Oct 5, 2023

@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from ab156ae to 6f31d7e Compare October 5, 2023 15:15
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from 6f31d7e to eaa99c0 Compare October 5, 2023 16:15
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Oct 5, 2023

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from eaa99c0 to 3e52605 Compare October 5, 2023 17:53
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you double-check the const char* handling of name for the cross-thread issues I asked about? r=me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could do these imports in the implementation files, since the new declaration just needs a definition for xpc_conneciton_t, which we could forward declare. I think these are fairly big headers, so avoiding them in this header could be a nice build-time benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point! I tried doing the forward declaration, although it became a bit complicated since the definition of xpc_connection_t depends on the language. If you are ok with it, I'll keep the include for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add the objc include headers here instead of in the AuxiliaryProcessExtensionBridge.h header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the objc header in the latest patch, since it was not required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Unneeded whitespace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto extra newline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in all 3 places in latest patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this maybe just be an Objective C NSDictionary, since it's only really needed for Cocoa APIs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, maybe not since the key is used for regular WebKit C++ code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ASCIILiteral instead of const char*, then thread-safety is not a concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point! I will change this to ASCIILiteral.

Thanks for reviewing!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of callOnMainRunLoop makes it seem like some of this code might run off the main thread. Do we need to be doing an isolated copy of name here? Or adding asserts that the other code referencing this name (which comes from serviceNameAndIdentifier) is on the right thread?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If manager does this work off the main thread, we might need to use isolated copy on name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for the other case sections.

@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2023
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from 3e52605 to c6eba03 Compare October 5, 2023 19:17
@pvollan
Copy link
Contributor Author

pvollan commented Oct 5, 2023

Looks good. Can you double-check the const char* handling of name for the cross-thread issues I asked about? r=me

That is a good point. In this case, I do not think we need to do an isolated copy, since name is immutable.

Thanks for reviewing!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use a local variable for the result of weakProcessLauncher.get(). Calling weakProcessLauncher.get() is as cheap as you might expect in this case. So calling it 3 times in a row is not ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point! Will fix in upcoming patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we WTFMove(process) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will fix in upcoming patch.

@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from c6eba03 to 6fda08a Compare October 5, 2023 21:47
@pvollan pvollan force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from 6fda08a to 7755c87 Compare October 5, 2023 22:28
@pvollan pvollan added the merge-queue Applied to send a pull request to merge-queue label Oct 6, 2023
https://bugs.webkit.org/show_bug.cgi?id=261966
rdar://problem/115906222

Reviewed by Brent Fulgham.

To benefit of the launch time improvements by launching WebKit processes as extensions, this patch implements
this approach by using ExtensionKit for launching. The feature is currently behind a flags, which is not
being turned on in this patch. The extensions are currently being installed in the WebKit framework.

* Source/WebKit/Configurations/BaseExtension.xcconfig:
* Source/WebKit/Platform/spi/Cocoa/ExtensionKitSPI.h: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h.
* Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:
* Source/WebKit/Scripts/process-entitlements.sh:
* Source/WebKit/Shared/API/Cocoa/WKMain.h:
* Source/WebKit/Shared/API/Cocoa/WKMain.mm:
(WKExtensionEventHandler):
(WKExtensionMain): Deleted.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.entitlements: Removed.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift:
(GPUProcessExtension.handle(_:)):
(AuxiliaryProcessExtension.main): Deleted.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h:
* Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.mm:
(handleNewConnection):
(extensionMain): Deleted.
* Source/WebKit/Shared/AuxiliaryProcessExtensions/GPUProcessExtension.swift: Renamed from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift.
(GPUProcessExtension.handle(_:)):
* Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift.
(NetworkingProcessExtension.handle(_:)):
* Source/WebKit/Shared/AuxiliaryProcessExtensions/WebContentProcessExtension.swift: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtension.swift.
(WebContentProcessExtension.handle(_:)):
* Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:
* Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:
(WebKit::XPCServiceEventHandler):
(WebKit::XPCServiceMain):
* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
* Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:
(WebKit::serviceNameAndIdentifier):
(WebKit::launchWithExtensionKit):
(WebKit::ProcessLauncher::launchProcess):
(WebKit::ProcessLauncher::finishLaunchingProcess):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/268990@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Improve-launch-time-of-WebKit-processes-v2 branch from 7755c87 to 9590b61 Compare October 6, 2023 14:58
@webkit-commit-queue
Copy link
Collaborator

Committed 268990@main (9590b61): https://commits.webkit.org/268990@main

Reviewed commits have been landed. Closing PR #18651 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 9590b61 into WebKit:main Oct 6, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants