fix: performance problem in crashReporter.start() on macOS
#34609
Conversation
a767c62 to
4216a27
Compare
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>
4216a27 to
f8ad716
Compare
|
Release Notes Persisted
|
|
I have automatically backported this PR to "17-x-y", please check out #34637 |
|
I have automatically backported this PR to "19-x-y", please check out #34638 |
|
I have automatically backported this PR to "20-x-y", please check out #34639 |
|
I have automatically backported this PR to "18-x-y", please check out #34640 |
|
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. https://chromium-review.googlesource.com/c/chromium/src/+/3727368 |
|
@becauseOf responded in #35592 (comment) |
|
@RaisinTen thanks, can you merge this commit? about reduce to 1ms，https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3711503 |
|
@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 :) |
…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>
Description of Change
This change reduces the duration of
crashReporter.start()on Intel macOSfrom 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
npm testpassesRelease Notes
Notes: Fixes a performance problem in
crashReporter.start()on macOS.