perf: patch libuv to use posix_spawn on macOS by pdesantis · Pull Request #27026 · electron/electron

pdesantis · 2020-12-15T19:29:22Z

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

PR description included and stakeholders cc'd
PATH env var breaking change is fixed
npm test passes
tests are changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed slow child process spawning on macOS Big Sur

welcome · 2020-12-15T19:29:24Z

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

nornagon · 2020-12-15T19:38:25Z

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 · 2020-12-15T19:49:04Z

@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:

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

nornagon · 2020-12-15T20:31:46Z

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

codebytere · 2021-01-20T22:04:08Z

@pdesantis do you still plan to work on this?

pdesantis · 2021-01-20T23:07:43Z

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

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 <[email protected]> Co-authored-by: Marcello Bastéa-Forte <[email protected]> Electron patch prepared by: Pat DeSantis <[email protected]>

pdesantis · 2021-01-25T21:41:24Z

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

nornagon

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

pdesantis · 2021-02-01T23:46:38Z

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 · 2021-02-04T19:00:15Z

@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 · 2021-02-05T09:25:45Z

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

welcome · 2021-02-08T15:25:07Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2021-02-08T15:25:08Z

Release Notes Persisted

Fixed slow child process spawning on macOS Big Sur

trop · 2021-02-08T15:26:03Z

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

trop · 2021-02-08T15:26:06Z

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

trop · 2021-02-08T15:26:09Z

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

This reverts commit f69c111.

* Revert "perf: patch libuv to use posix_spawn on macOS (#27026)" This reverts commit f69c111. * Update .patches Co-authored-by: VerteDinde <[email protected]> Co-authored-by: Robo <[email protected]>

…) (electron#27810)" This reverts commit a859436.

electron-cation bot added the new-pr 🌱 PR opened recently label Dec 15, 2020

pdesantis force-pushed the master-macos-libuv-use-posix-spawn branch from 35bfc3f to c371770 Compare December 15, 2020 19:31

pdesantis mentioned this pull request Dec 15, 2020

child_process spawn locks up renderer for 300 - 3000ms on signed app on macOS Big Sur #26143

Closed

3 tasks

electron-cation bot removed the new-pr 🌱 PR opened recently label Dec 22, 2020

codebytere added the semver/patch backwards-compatible bug fixes label Jan 20, 2021

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

pdesantis force-pushed the master-macos-libuv-use-posix-spawn branch from c371770 to 16370a2 Compare January 25, 2021 21:01

Remove trailing whitespaces from patch file

7c1d521

pdesantis force-pushed the master-macos-libuv-use-posix-spawn branch from 16dff6f to 7c1d521 Compare January 25, 2021 21:28

pdesantis marked this pull request as ready for review January 25, 2021 21:32

pdesantis requested a review from a team as a code owner January 25, 2021 21:32

deepak1556 added target/10-x-y labels Jan 25, 2021

deepak1556 mentioned this pull request Jan 27, 2021

VSCode Insdier for ARM64 on M1 Macmini SOURCE CONTROL is slowly microsoft/vscode#113660

Closed

marcello3d mentioned this pull request Jan 27, 2021

Integrated Terminal Lags Intermittently microsoft/vscode#105446

Closed

deepak1556 mentioned this pull request Jan 31, 2021

VSCode randomly semi-freezes microsoft/vscode#112269

Closed

nornagon approved these changes Feb 1, 2021

View reviewed changes

update patches

8c24d75

This was referenced Feb 8, 2021

perf: patch libuv to use posix_spawn on macOS #27654

Merged

perf: patch libuv to use posix_spawn on macOS #27655

Merged

perf: patch libuv to use posix_spawn on macOS #27656

Merged

trop bot added in-flight/11-x-y and removed target/11-x-y labels Feb 8, 2021

trop bot removed the target/10-x-y label Feb 8, 2021

trop bot added in-flight/10-x-y and removed target/12-x-y labels Feb 8, 2021

VerteDinde added a commit that referenced this pull request Feb 18, 2021

Revert "perf: patch libuv to use posix_spawn on macOS (#27026)"

77525ac

This reverts commit f69c111.

VerteDinde added a commit that referenced this pull request Feb 18, 2021

Revert "perf: patch libuv to use posix_spawn on macOS (#27026)"

f2bc744

This reverts commit f69c111.

deepak1556 pushed a commit that referenced this pull request Feb 18, 2021

Revert "perf: patch libuv to use posix_spawn on macOS (#27026)" (#27797)

6a9ba42

This reverts commit f69c111.

trop bot pushed a commit that referenced this pull request Feb 18, 2021

Revert "perf: patch libuv to use posix_spawn on macOS (#27026)"

ce861dd

This reverts commit f69c111.

trop bot pushed a commit that referenced this pull request Feb 18, 2021

Revert "perf: patch libuv to use posix_spawn on macOS (#27026)"

0c61bb5

This reverts commit f69c111.

timsu pushed a commit to cryptagon/electron that referenced this pull request May 13, 2021

Revert "revert: patch libuv to use posix_spawn on macOS (electron#27026…

1fc9564

…) (electron#27810)" This reverts commit a859436.

dreamerns pushed a commit to cryptagon/electron that referenced this pull request Aug 17, 2021

Revert "revert: patch libuv to use posix_spawn on macOS (electron#27026…

68abda9

…) (electron#27810)" This reverts commit a859436.

dreamerns pushed a commit to cryptagon/electron that referenced this pull request Sep 14, 2021

Revert "revert: patch libuv to use posix_spawn on macOS (electron#27026…

cf60727

…) (electron#27810)" This reverts commit a859436.

Conversation

pdesantis commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Special thanks

Stakeholders

Checklist

Release Notes

Uh oh!

welcome bot commented Dec 15, 2020

Uh oh!

nornagon commented Dec 15, 2020

Uh oh!

pdesantis commented Dec 15, 2020

Uh oh!

nornagon commented Dec 15, 2020

Uh oh!

codebytere commented Jan 20, 2021

Uh oh!

pdesantis commented Jan 20, 2021

Uh oh!

pdesantis commented Jan 25, 2021

Uh oh!

nornagon left a comment

Choose a reason for hiding this comment

Uh oh!

pdesantis commented Feb 1, 2021

Uh oh!

jkleinsc commented Feb 4, 2021

Uh oh!

andrejgloot commented Feb 5, 2021

Uh oh!

welcome bot commented Feb 8, 2021

Uh oh!

release-clerk bot commented Feb 8, 2021

Uh oh!

trop bot commented Feb 8, 2021

Uh oh!

trop bot commented Feb 8, 2021

Uh oh!

trop bot commented Feb 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pdesantis commented Dec 15, 2020 •

edited

Loading