Skip to content
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.

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

perf: patch libuv to use posix_spawn on macOS #27026

Merged

Conversation

pdesantis
Copy link
Contributor

@pdesantis pdesantis commented Dec 15, 2020

Description of Change

Spawning child processes in an Electron application with a hardened runtime has become slow in macOS Big Sur.

This patch is a squashed version of libuv/libuv#3064, and should be removed when the libuv PR is merged.

Fixes: libuv/libuv#3050
Fixes: #26143
PR-URL: libuv/libuv#3064

Special thanks

@jpcanepa: authoring the libuv patch
@davidzech @marcello3d @srubin: identifying the underlying Electron issue
@marcello3d: creating the Electron repro
@bnoordhuis: assistance with libuv

Stakeholders

@deepak1556: thanks for the libuv repro
@erickzhao: thanks for the support in the Electron Discord & assisting with Electron build-tools

Checklist

Release Notes

Notes: Fixed slow child process spawning on macOS Big Sur

@welcome
Copy link

welcome bot commented Dec 15, 2020

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 15, 2020
@pdesantis pdesantis force-pushed the master-macos-libuv-use-posix-spawn branch from 35bfc3f to c371770 Compare December 15, 2020 19:31
@nornagon
Copy link
Member

Am I correct in understanding that the breaking change is that previously, the PATH in the env option was used to resolve the executable, but now the PATH of the parent process is used? That seems potentially dangerous, but I'll leave that to the libuv maintainers to comment on. We should hold on merging this until we clarify whether we feel OK about merging that breaking change or if there's a way to restore the old behavior and make this non-breaking.

@pdesantis
Copy link
Contributor Author

@nornagon thanks for taking a look! Correct, that's the breaking change I was referring to (I'll update the description to clarify). In my opinion this should not be merged until that is fixed. My motivations for the draft PR are:

  1. This has fixed the Big Sur issue for us, hopefully some others in the community find it useful.
  2. Once the blocker is resolved, is this the correct way to patch Electron?

@nornagon
Copy link
Member

@pdesantis yes, this is the right way to land a patch in Electron.

@codebytere
Copy link
Member

@pdesantis do you still plan to work on this?

@codebytere codebytere added the semver/patch backwards-compatible bug fixes label Jan 20, 2021
@pdesantis
Copy link
Contributor Author

@codebytere yes! I'm just waiting on final approval for libuv/libuv#3064 (which we submitted yesterday), then will update this PR.

@pdesantis pdesantis changed the title fix: patch libuv to use posix_spawn on macOS perf: patch libuv to use posix_spawn on macOS Jan 25, 2021
patch libuv to fix a performance regression in macOS >= 11

Spawning child processes in an Electron application with a hardened
runtime has become slow in macOS Big Sur. This patch is a squashed
version of libuv/libuv#3064

This patch should be removed when libuv PR 3064 is merged.

Fixes: libuv/libuv#3050
Fixes: electron#26143
PR-URL: libuv/libuv#3064

Authored-by: Juan Pablo Canepa <jpcanepa@gmail.com>
Co-authored-by: Marcello Bastéa-Forte <marcello@descript.com>
Electron patch prepared by: Pat DeSantis <pdesantis3@gmail.com>
@pdesantis pdesantis force-pushed the master-macos-libuv-use-posix-spawn branch from c371770 to 16370a2 Compare January 25, 2021 21:01
@pdesantis pdesantis force-pushed the master-macos-libuv-use-posix-spawn branch from 16dff6f to 7c1d521 Compare January 25, 2021 21:28
@pdesantis pdesantis marked this pull request as ready for review January 25, 2021 21:32
@pdesantis pdesantis requested a review from a team as a code owner January 25, 2021 21:32
@pdesantis
Copy link
Contributor Author

@codebytere @nornagon I'm marking this as Ready for Review now. We fixed the aforementioned breaking change and have received 1 merge approval on libuv PR libuv/libuv#3064 (but it has not been merged yet).

I'm guessing you want to wait for libuv to merge the PR, but figured now is a good time to start the Electron review process.

I had to make 1 change to the patch (stripping trailing whitespace) to pass Electron's lint. I can squash this down into 1 commit once the review is done.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

I'm OK merging / backporting this once it's merged upstream.

@pdesantis
Copy link
Contributor Author

I'm OK merging / backporting this once it's merged upstream.

Awesome, I'll report back once it's merged. In the meantime, what should I do about these test failures?

@jkleinsc
Copy link
Contributor

jkleinsc commented Feb 4, 2021

@pdesantis the patch needs to be updated. If you are using electron build tools, you can run e patches node on your local checkout which should update the patch locally. Alternatively, you can go to one of the failed jobs, eg https://app.circleci.com/pipelines/github/electron/electron/35194/workflows/fe990252-b819-4d60-91ce-833802f7b2ee/jobs/771654, click on the Artifacts tab, and download the file patches/update-patches.patch to your electron dir and then run git am update-patches.patch

@andrejgloot
Copy link

libuv/libuv#3064 PR has been merged and closed now

@welcome
Copy link

welcome bot commented Feb 8, 2021

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link

release-clerk bot commented Feb 8, 2021

Release Notes Persisted

Fixed slow child process spawning on macOS Big Sur

@trop
Copy link
Contributor

trop bot commented Feb 8, 2021

I have automatically backported this PR to "11-x-y", please check out #27654

@trop
Copy link
Contributor

trop bot commented Feb 8, 2021

I have automatically backported this PR to "10-x-y", please check out #27655

@trop trop bot removed the target/10-x-y label Feb 8, 2021
@trop
Copy link
Contributor

trop bot commented Feb 8, 2021

I have automatically backported this PR to "12-x-y", please check out #27656

VerteDinde added a commit that referenced this pull request Feb 18, 2021
VerteDinde added a commit that referenced this pull request Feb 18, 2021
deepak1556 pushed a commit that referenced this pull request Feb 18, 2021
trop bot pushed a commit that referenced this pull request Feb 18, 2021
trop bot pushed a commit that referenced this pull request Feb 18, 2021
codebytere pushed a commit that referenced this pull request Feb 19, 2021
* Revert "perf: patch libuv to use posix_spawn on macOS (#27026)"

This reverts commit f69c111.

* Update .patches

Co-authored-by: VerteDinde <khammond@slack-corp.com>
Co-authored-by: Robo <hop2deep@gmail.com>
jkleinsc pushed a commit that referenced this pull request Feb 22, 2021
* Revert "perf: patch libuv to use posix_spawn on macOS (#27026)"

This reverts commit f69c111.

* Update .patches

Co-authored-by: VerteDinde <khammond@slack-corp.com>
Co-authored-by: Robo <hop2deep@gmail.com>
timsu pushed a commit to cryptagon/electron that referenced this pull request May 13, 2021
dreamerns pushed a commit to cryptagon/electron that referenced this pull request Aug 17, 2021
dreamerns pushed a commit to cryptagon/electron that referenced this pull request Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
7 participants