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

fix(perf): sign multiple files at once #347

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erikian
Copy link
Member

@erikian erikian commented Feb 16, 2025

I was recently debugging a code-signing issue and noticed in the logs that we're signing one file at a time, which translates to a few hundreds of childProcess.exec('codesign') calls. codesign supports passing multiple files at once, so we can take advantage of that to keep the number of child processes we have to spawn (and the associated overhead) to a minimum.

At first, I tried to sign all files with identical args with a single codesign call, but the tests for the v7.0.0-beta.3-darwin-x64 and v7.0.0-beta.3-mas-x64 artifacts were failing. I figured that was related to fact that that we're supposed to sign the bundle from the inside out as described here, so I changed the logic to take into account the position of the files in the file tree, and now all files at the same depth that share the same arguments are signed at the same time.

I did some crude benchmarking on my machine with the Electron artifacts used in @electron/osx-sign's test cases by console.timeing the total time spent and counting how many child processes we're spawning. Using the main branch, the heaviest artifact is v6.0.3-darwin-x64, which takes 55.737s to complete and calls codesign 227 times in total — with my branch, the same artifact is signed in 47.938s (~14% less time) while spawning only 13 child processes.

I didn't realize we had an open PR for improving code-signing performance until just before opening this one. @SuperDisk, I couldn't test your branch locally because of some build errors, but if my PR lands before yours let me know if it helps with your use case. My PR uses a different approach, so if it doesn't solve your problem completely it could potentially speed things up some more when combined with your changes.

Closes #305.

@erikian erikian requested a review from a team as a code owner February 16, 2025 19:45
@SuperDisk
Copy link

SuperDisk commented Feb 17, 2025

Looks like this does essentially the same thing as mine, although it uses one invocation per "level" rather than spawning multiple codesign processes; I think it probably will address the same use case. Nice work, if either of our PRs gets merged I'll be happy.

EDIT: that said, my thing fixes an unrelated bug where a stat should have been an lstat, so if we do merge this then I can put up another PR with that fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codesign all files in one step
2 participants