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.

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

fix: performance problem in crashReporter.start() on macOS #34609

Merged
merged 1 commit into from Jun 20, 2022
Merged

fix: performance problem in crashReporter.start() on macOS #34609

merged 1 commit into from Jun 20, 2022

Conversation

RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Jun 17, 2022 •
edited

Description of Change

This change reduces the duration of crashReporter.start() on Intel macOS
from 622 milliseconds to 257 milliseconds!

Backports https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386

posix: Replace DoubleForkAndExec() with ForkAndSpawn()

The DoubleForkAndExec() function was taking over 622 milliseconds to run
on macOS 11 (BigSur) on Intel i5-1038NG7. I did some debugging by adding
some custom traces and found that the fork() syscall is the bottleneck
here, i.e., the first fork() takes around 359 milliseconds and the
nested fork() takes around 263 milliseconds. Replacing the nested fork()
and exec() with posix_spawn() reduces the time consumption to 257
milliseconds!

See libuv/libuv#3064 to know why fork() is so
slow on macOS and why posix_spawn() is a better replacement.

Another point to note is that even base::LaunchProcess() from Chromium
calls posix_spawnp() on macOS -
https://source.chromium.org/chromium/chromium/src/+/8f8d82dea0fa8f11f57c74dbb65126f8daba58f7:base/process/launch_mac.cc;l=295-296

Change-Id: I25c6ee9629a1ae5d0c32b361b56a1ce0b4b0fd26
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386
Reviewed-by: Mark Mentovai mark@chromium.org
Commit-Queue: Mark Mentovai mark@chromium.org

Fixes: #34321
Signed-off-by: Darshan Sen raisinten@gmail.com

Checklist

Release Notes

Notes: Fixes a performance problem in crashReporter.start() on macOS.

@RaisinTen RaisinTen requested review from a team as code owners June 17, 2022 08:09
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 17, 2022
@RaisinTen RaisinTen mentioned this pull request Jun 17, 2022
Closed
3 tasks
@RaisinTen RaisinTen force-pushed the reduce-crash-reporter-init-duration branch from a767c62 to 4216a27 Compare June 17, 2022 08:14
@dsanders11 dsanders11 added the semver/minor backwards-compatible functionality label Jun 17, 2022
@dsanders11 dsanders11 added the semver/patch backwards-compatible bug fixes label Jun 17, 2022
@RaisinTen
fix: performance problem in crashReporter.start() on macOS
f8ad716 
This change reduces the duration of crashReporter.start() on Intel macOS
from 622 milliseconds to 257 milliseconds!

Backports https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386

  posix: Replace DoubleForkAndExec() with ForkAndSpawn()

  The DoubleForkAndExec() function was taking over 622 milliseconds to run
  on macOS 11 (BigSur) on Intel i5-1038NG7. I did some debugging by adding
  some custom traces and found that the fork() syscall is the bottleneck
  here, i.e., the first fork() takes around 359 milliseconds and the
  nested fork() takes around 263 milliseconds. Replacing the nested fork()
  and exec() with posix_spawn() reduces the time consumption to 257
  milliseconds!

  See libuv/libuv#3064 to know why fork() is so
  slow on macOS and why posix_spawn() is a better replacement.

  Another point to note is that even base::LaunchProcess() from Chromium
  calls posix_spawnp() on macOS -
  https://source.chromium.org/chromium/chromium/src/+/8f8d82dea0fa8f11f57c74dbb65126f8daba58f7:base/process/launch_mac.cc;l=295-296

  Change-Id: I25c6ee9629a1ae5d0c32b361b56a1ce0b4b0fd26
  Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386
  Reviewed-by: Mark Mentovai <mark@chromium.org>
  Commit-Queue: Mark Mentovai <mark@chromium.org>

Fixes: electron#34321
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the reduce-crash-reporter-init-duration branch from 4216a27 to f8ad716 Compare June 17, 2022 10:26
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 18, 2022
@zcbenz zcbenz merged commit ec98e95 into electron:main Jun 20, 2022
@release-clerk
Copy link

release-clerk bot commented Jun 20, 2022

Release Notes Persisted

Fixes a performance problem in crashReporter.start() on macOS.

This was referenced Jun 20, 2022
Merged
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Jun 20, 2022

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

@trop
Copy link
Contributor

trop bot commented Jun 20, 2022

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

@trop trop bot removed the target/17-x-y label Jun 20, 2022
@trop
Copy link
Contributor

trop bot commented Jun 20, 2022

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

@trop
Copy link
Contributor

trop bot commented Jun 20, 2022

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

@RaisinTen RaisinTen deleted the reduce-crash-reporter-init-duration branch June 20, 2022 04:52
@bpasero bpasero mentioned this pull request Jul 22, 2022
Closed
@commit-lint commit-lint bot mentioned this pull request Aug 19, 2022
Open
@becauseOf
Copy link

becauseOf commented Sep 7, 2022 •
edited

this was reverted, can you fix it?

Revert "posix: Replace DoubleForkAndExec() with ForkAndSpawn()"

This reverts commit 460943dd9a71dc76f68182a8ede766d5543e5341.

Reason for revert: This fails to compile in Chromium Android.
posix_spawn and posix_spawnp are available in Android NDK 28, but
Chromium is building with version 23.

https://chromium-review.googlesource.com/c/chromium/src/+/3727368

@becauseOf becauseOf mentioned this pull request Sep 7, 2022
Closed
3 tasks
@RaisinTen
Copy link
Member Author

@becauseOf responded in #35592 (comment)

@becauseOf
Copy link

becauseOf commented Sep 8, 2022 •
edited

@RaisinTen thanks, can you merge this commit? about reduce to 1ms，https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3711503

@RaisinTen
Copy link
Member Author

@becauseOf I can't merge it in its current state because of the reasons stated in the review comments but I do plan to get back to that CL and bring it to a workable state when I have more time :)

@RaisinTen RaisinTen mentioned this pull request Oct 25, 2022
Closed
3 tasks
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@RaisinTen @khalwa
fix: performance problem in crashReporter.start() on macOS (electro…
ee4ae1d 
…n#34609)

fix: performance problem in crashReporter.start() on macOS

This change reduces the duration of crashReporter.start() on Intel macOS
from 622 milliseconds to 257 milliseconds!

Backports https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386

  posix: Replace DoubleForkAndExec() with ForkAndSpawn()

  The DoubleForkAndExec() function was taking over 622 milliseconds to run
  on macOS 11 (BigSur) on Intel i5-1038NG7. I did some debugging by adding
  some custom traces and found that the fork() syscall is the bottleneck
  here, i.e., the first fork() takes around 359 milliseconds and the
  nested fork() takes around 263 milliseconds. Replacing the nested fork()
  and exec() with posix_spawn() reduces the time consumption to 257
  milliseconds!

  See libuv/libuv#3064 to know why fork() is so
  slow on macOS and why posix_spawn() is a better replacement.

  Another point to note is that even base::LaunchProcess() from Chromium
  calls posix_spawnp() on macOS -
  https://source.chromium.org/chromium/chromium/src/+/8f8d82dea0fa8f11f57c74dbb65126f8daba58f7:base/process/launch_mac.cc;l=295-296

  Change-Id: I25c6ee9629a1ae5d0c32b361b56a1ce0b4b0fd26
  Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386
  Reviewed-by: Mark Mentovai <mark@chromium.org>
  Commit-Queue: Mark Mentovai <mark@chromium.org>

Fixes: electron#34321
Signed-off-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: crashReporter.start blocks main process takes more than 600ms on macOS
4 participants
@RaisinTen @becauseOf @zcbenz @dsanders11