Skip to content

REGRESSION(281488@main): [WPE][GTK] Process launching is slow #33270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 10, 2024
Merged

REGRESSION(281488@main): [WPE][GTK] Process launching is slow #33270

merged 1 commit into from
Sep 10, 2024

Conversation

mcatanzaro
Copy link
Contributor

@mcatanzaro mcatanzaro commented Sep 7, 2024 •
edited by webkit-early-warning-system
Loading

cf8f5f5

REGRESSION(281488@main): [WPE][GTK] Process launching is slow
https://bugs.webkit.org/show_bug.cgi?id=279026

Reviewed by Carlos Garcia Campos and Adrian Perez de Castro.

ProcessLauncher is already designed to support asynchronous process
launching, but the GLib implementation ignores this and does everything
synchronously. Previously this was OK because
ProcessLauncher::launchProcess would return immediately after the
subprocess is spawned, without waiting for any code to execute in the
subprocess. Apparently that's fast enough in practice. But after my
changes in 281488@main, we now additionally wait for the subprocess to
send credentials to the parent via a socket. That turns out to be
drastically slower than actually spawning the process.

The blocking is unnecessary because the code was already carefully
structured to allow asynchronicity. I just failed to take advantage of
it.

Instead, return immediately after spawning the subprocess. Then, wait
until the socket is ready before reading the pid from it. Now the read
can complete instantaneously instead of blocking the UI process.

* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
* Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:
(WebKit::ProcessLauncher::launchProcess):

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

5768449

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt ✅ 🛠 wpe-cairo
  🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2   🧪 mac-intel-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@mcatanzaro mcatanzaro requested review from a team and cdumez as code owners September 7, 2024 00:16
@mcatanzaro mcatanzaro self-assigned this Sep 7, 2024
@mcatanzaro mcatanzaro added the WebKitGTK Bugs related to the Gtk API layer. label Sep 7, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 7, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 68a3b45)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style   🛠 ios   🛠 mac   🛠 wpe   🛠 wincairo
  🛠 ios-sim   🛠 mac-AS-debug   🧪 wpe-wk2   🧪 wincairo-tests
  🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2   🛠 gtk
  🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
  🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2
  🛠 tv
  🛠 tv-sim
  🛠 watch
  🛠 watch-sim

@mcatanzaro mcatanzaro changed the title REGRESSION(281488@main): Process launching is slow under flatpak REGRESSION(281488@main): Process launching is slow Sep 7, 2024
@mcatanzaro mcatanzaro force-pushed the eng/REGRESSION281488main-Process-launching-is-slow-under-flatpak branch from 68a3b45 to 7ac3294 Compare September 7, 2024 00:16
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 7, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 7ac3294)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style   🛠 ios   🛠 mac   🛠 wpe   🛠 wincairo
  🛠 ios-sim   🛠 mac-AS-debug   🧪 wpe-wk2   🧪 wincairo-tests
  🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2   🛠 gtk
  🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
  🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2
  🛠 tv
  🛠 tv-sim
  🛠 watch
  🛠 watch-sim

@mcatanzaro mcatanzaro changed the title REGRESSION(281488@main): Process launching is slow REGRESSION(281488@main): [WPE][GTK] Process launching is slow Sep 7, 2024
@mcatanzaro mcatanzaro force-pushed the eng/REGRESSION281488main-Process-launching-is-slow-under-flatpak branch from 7ac3294 to 77d5936 Compare September 7, 2024 00:17
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 7, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 77d5936)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac   🛠 wpe   🛠 wincairo
  🛠 ios-sim   🛠 mac-AS-debug   🧪 wpe-wk2   🧪 wincairo-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt ⏳ 🧪 mac-wk1   🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2   🛠 gtk
  🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
  🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
⏳ 🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2
  🛠 tv
  🛠 tv-sim
  🛠 watch
  🛠 watch-sim

@mcatanzaro mcatanzaro force-pushed the eng/REGRESSION281488main-Process-launching-is-slow-under-flatpak branch from 77d5936 to f210f5e Compare September 7, 2024 00:21
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 7, 2024 •
edited
Loading

EWS run on previous version of this PR (hash f210f5e)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 7, 2024
carlosgcampos
carlosgcampos approved these changes Sep 9, 2024
View reviewed changes
aperezdc
aperezdc approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

Nice that we can make process launching fully async. Probably it would have been good to have even before the recent change to pass real PIDs to the launcher—better late than never!

@mcatanzaro
Copy link
Contributor Author

Nice that we can make process launching fully async. Probably it would have been good to have even before the recent change to pass real PIDs to the launcher—better late than never!

I'm not sure that it's actually fully async. It's using g_subprocess_launcher_spawnv() and that looks synchronous to me. It just happens to be fast enough that wasn't a problem in practice until 281488@main.

@mcatanzaro mcatanzaro removed the merging-blocked Applied to prevent a change from being merged label Sep 9, 2024
@mcatanzaro mcatanzaro force-pushed the eng/REGRESSION281488main-Process-launching-is-slow-under-flatpak branch from f210f5e to d95ef65 Compare September 9, 2024 15:54
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 9, 2024 •
edited
Loading

EWS run on previous version of this PR (hash d95ef65)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@mcatanzaro mcatanzaro force-pushed the eng/REGRESSION281488main-Process-launching-is-slow-under-flatpak branch from d95ef65 to 5768449 Compare September 10, 2024 14:58
@mcatanzaro mcatanzaro added the merge-queue Applied to send a pull request to merge-queue label Sep 10, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 10, 2024 •
edited
Loading

EWS run on current version of this PR (hash 5768449)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt ✅ 🛠 wpe-cairo
  🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2   🧪 mac-intel-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@mcatanzaro
REGRESSION(281488@main): [WPE][GTK] Process launching is slow
cf8f5f5 
https://bugs.webkit.org/show_bug.cgi?id=279026

Reviewed by Carlos Garcia Campos and Adrian Perez de Castro.

ProcessLauncher is already designed to support asynchronous process
launching, but the GLib implementation ignores this and does everything
synchronously. Previously this was OK because
ProcessLauncher::launchProcess would return immediately after the
subprocess is spawned, without waiting for any code to execute in the
subprocess. Apparently that's fast enough in practice. But after my
changes in 281488@main, we now additionally wait for the subprocess to
send credentials to the parent via a socket. That turns out to be
drastically slower than actually spawning the process.

The blocking is unnecessary because the code was already carefully
structured to allow asynchronicity. I just failed to take advantage of
it.

Instead, return immediately after spawning the subprocess. Then, wait
until the socket is ready before reading the pid from it. Now the read
can complete instantaneously instead of blocking the UI process.

* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
* Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:
(WebKit::ProcessLauncher::launchProcess):

Canonical link: https://commits.webkit.org/283414@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/REGRESSION281488main-Process-launching-is-slow-under-flatpak branch from 5768449 to cf8f5f5 Compare September 10, 2024 15:29
@webkit-commit-queue
Copy link
Collaborator

Committed 283414@main (cf8f5f5): https://commits.webkit.org/283414@main

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

@webkit-commit-queue webkit-commit-queue merged commit cf8f5f5 into WebKit:main Sep 10, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aperezdc aperezdc aperezdc approved these changes

@carlosgcampos carlosgcampos carlosgcampos approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@mcatanzaro mcatanzaro

Labels
WebKitGTK Bugs related to the Gtk API layer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mcatanzaro @webkit-early-warning-system @webkit-commit-queue @aperezdc @carlosgcampos @webkit-ews-buildbot