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

Speed up Ghost boot time #10758

Merged
merged 2 commits into from Jun 21, 2019

Conversation

Projects
None yet
4 participants
@daniellockyer
Copy link
Contributor

commented May 25, 2019

Speed up Ghost boot time by replacing the JS-only RSA key generator library with a native alternative.


After profiling the initial boot, I saw the key generation library took around 25% of the boot time. (Flat section in the middle with red at the top.) JS-only libraries tend to be pretty slow for crypto.

image

I benchmarked some alternatives and rsa-keypair (a native lib) is much faster.

keypair x 3.99 ops/sec ±34.45% (15 runs sampled)
rsa-keypair x 48.53 ops/sec ±8.31% (23 runs sampled)
Fastest is rsa-keypair

Server (in development mode) boot time on my laptop:

  • Before: ~2.1s
  • After: ~1.7s

So it looks like it's about 20% faster for me.

Just required a quick switch-out but should work the same. 👍

@daniellockyer daniellockyer force-pushed the daniellockyer:boot-time branch from 3f27663 to 9855028 May 26, 2019

@kevinansfield

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Hey @neosilky 👋 Thanks for this, looks really interesting! In general we've tried to keep away from native/binary dependencies because they've historically caused a lot of install problems for users across the myriad of environments in which Ghost is used. That said, startup performance is important to us so I've raised this internally to review in more detail 🙂

@kevinansfield

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Looking at where keypair is used it looks like there may be a potential refactoring to skip it altogether on boot.

The dynamicDefault object is used to populate default settings values for when they don't already exist, however it's still generated on every boot. Normally this isn't much of an issue because it's really quick but the usage of keypair has changed that. It may be that we can refactor the members keypair generation so that it only happens once on first boot, or we can move the generation to where it's used and only run it if the keypair is not already stored in settings. /cc @allouis, @rishabhgrg

@daniellockyer

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@kevinansfield Sure thing! The library only relies on OpenSSL but maybe there are situations where this could cause a problem.

I do have some other changes that would help performance. They're just JavaScript changes but mostly requires switching pretty lodash code for uglier vanilla code. Probably not something you want to do but it's an option.

@daniellockyer

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@kevinansfield Even better! 😄

@kevinansfield

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

I do have some other changes that would help performance. They're just JavaScript changes but mostly requires switching pretty lodash code for uglier vanilla code. Probably not something you want to do but it's an option.

If they have a measurable impact (especially on boot time) and the code isn't obscured too much then we're definitely open to those sorts of contributions 🙂

@daniellockyer

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

If they have a measurable impact (especially on boot time) and the code isn't obscured too much then we're definitely open to those sorts of contributions slightly_smiling_face

Ok cool, will see what I have 👍

@rishabhgrg

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@kevinansfield Raised an issue here to track update for members keypair generation, overall agree its a good idea to move keypair away from boot if possible at best, or only on first boot at worst. Good catch, thanks 😄

@allouis

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Hey @neosilky Only just got round to catching up with this.

@kevinansfield @rishabhgrg This looks like a sane and solid change for this - I think we should merge it regardless of updates to keypair generation being skipped.

@rishabhgrg I have moved that issue to this repo as the changes concern files in here 👍

@daniellockyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@allouis There have been a couple of updates to the new library since I pushed. Would you like me to update my PR and re-push?

@allouis

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@neosilky That would be great, afterwards I can test and get this in ☺️

@daniellockyer daniellockyer force-pushed the daniellockyer:boot-time branch from 9855028 to 6e187d8 Jun 21, 2019

@daniellockyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@neosilky That would be great, afterwards I can test and get this in relaxed

Should be good to go!

@allouis

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Awesome! Thanks 💃

@allouis allouis merged commit 6473569 into TryGhost:master Jun 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daniellockyer daniellockyer deleted the daniellockyer:boot-time branch Jun 21, 2019

@allouis

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Yo @neosilky thanks again for this PR (and the research/stats behind it!)

My laptop must be a bit slower than yours, because my boot time went from ~4.5 seconds to ~2.2 seconds! This has made my life so much better 😅 👻

@daniellockyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

Yo @neosilky thanks again for this PR (and the research/stats behind it!)

My laptop must be a bit slower than yours, because my boot time went from ~4.5 seconds to ~2.2 seconds! This has made my life so much better sweat_smile ghost

Haha - excellent! Would love to hear if it made much of a difference for your Pro platform.

Need to get around to sending in my other changes... It's a fairly minimal speedup for me, but now I know it's even more beneficial on other machines, they should still be useful.

kevinansfield added a commit that referenced this pull request Jun 26, 2019

Revert "Replaced keypair with rsa-keypair module (#10758)"
This reverts commit 6473569.

- `rsa-keypair` is a binary dependency that was failing to install for a lot of users, reverting for now so we can look at alternative options for speeding up boot time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.