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

Speed up new command by caching dependencies locally #2080

Merged
merged 18 commits into from
Sep 6, 2022
Merged

Speed up new command by caching dependencies locally #2080

merged 18 commits into from
Sep 6, 2022

Conversation

joshuayoes
Copy link
Contributor

@joshuayoes joshuayoes commented Aug 29, 2022 •
edited
Loading

Please verify the following:

  • yarn ci:test jest tests pass with new tests, if relevant
  • README.md has been updated with your changes, if relevant

Describe your PR

Problem

ignite-cli new with the --install-deps option is pretty slow. This makes it hard to test because we need to wait 1-2 minutes between each run to test changes. Frank has filed a few regressions (1, 2) after we have added flags. It would be nice to improve the install speed so iterating on changes in manual QA or in tests would be faster.

Potential Solutions

Cache dependencies

We can take the turbo repo approach where we copy all the folders and files after an install to a local file system cache, and then copy them to the boilerplate target on subsequent installs

@joshuayoes joshuayoes added this to the Ignite v8: Maverick milestone Aug 29, 2022
@joshuayoes joshuayoes self-assigned this Aug 29, 2022
jamonholmgren
jamonholmgren reviewed Aug 29, 2022
View reviewed changes
Copy link
Member

@jamonholmgren jamonholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction you're going! Some comments.

src/commands/new.ts Outdated Show resolved Hide resolved
src/commands/new.ts Outdated Show resolved Hide resolved
@mazenchami mazenchami added the maverick Ignite v8: Maverick label Aug 31, 2022
jamonholmgren
jamonholmgren reviewed Aug 31, 2022
View reviewed changes
Copy link
Member

@jamonholmgren jamonholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small comment!

src/tools/cache.ts Outdated Show resolved Hide resolved
@joshuayoes
Copy link
Contributor Author

CI Benchmark

  1. Cache hit
  2. Cache miss

This is faster locally, but I'm pretty happy with shaving off 1-3 minutes

@joshuayoes
Copy link
Contributor Author

New ignite cache command is done 👀

ignite cache help command output

@joshuayoes
Copy link
Contributor Author

On my local machine, ignite-cli new is now 5x times faster with a warm cache 🐎

Warm Cache: ~10s

Logo of ignite-cli new with warm cache

No Cache: ~55s

Screen Shot 2022-09-01 at 3 35 13 PM

@joshuayoes joshuayoes marked this pull request as ready for review September 2, 2022 00:08
@joshuayoes
feat(new): cache dependencies
7d8272a
@joshuayoes
fix(new): make sure cache directories exist
53700b5
@joshuayoes
feat(new): refactor copy logic into cache tool
0e8743a
@joshuayoes
fix(new): filter targets based on platform
9a60480
@joshuayoes
feat(cache): add cache.rootdir to simplify cache folder lookup
e892fd0
@joshuayoes
refactor(cache): add default plaform arg to copy
e71ad4b
@joshuayoes
feat(new): update cache message
1b3bc88
@joshuayoes
feat(.circleci): save and restore ignite deps cache between runs
e7b98ea
@joshuayoes
refactor(cache): remove builder pattern from tool
694454c
@joshuayoes
feat(new): only install deps if there is no cache
83cf0b8
@joshuayoes
feat(.circleci): add ignite deps cache key
36d8376
@joshuayoes
fix(.circleci): add stricter cache key
bd307b3
@joshuayoes
feat(commands): add cache command for interacting with cache
0b8a7ae
@joshuayoes
feat(new): add --use-cache flag
c6150ac
@joshuayoes
fix(new): choose cache strategy
9c477d1
@joshuayoes
fix(new): only cache artifacts if cache doesn't exist
f89ad63
@joshuayoes
Copy link
Contributor Author

I've recorded a video talking about the PR, why I decided to implement it, some of my reasoning: https://www.loom.com/share/abe0dd5852df42a7b1fba1c5bf848fec

@joshuayoes
Copy link
Contributor Author

Now you can go from ignite new to yarn expo:start in a minute: https://www.loom.com/share/edaca7d62eae4e5280f57e885c44a01c

@cdanwards
Copy link
Member

Took it for a spin and I'm getting an error on the second app (First using the cache) that I try to spin up:
Screen Shot 2022-09-02 at 4 21 25 PM

@joshuayoes
Copy link
Contributor Author

joshuayoes commented Sep 6, 2022 •
edited
Loading

Took it for a spin and I'm getting an error on the second app (First using the cache) that I try to spin up: Screen Shot 2022-09-02 at 4 21 25 PM

Thank you for catching this 🙏 I will re-create it locally and push up a fix

@joshuayoes
Copy link
Contributor Author

Screen Shot 2022-09-06 at 10 19 43 AM

@cdanwards would you mind verifying you did that with the latest commit on origin feat/new-perf? I'm on e612ae01448bc62b907bc06fbd1751e4d59e3d97 and I am not having that issue again.

I realized a few commits ago, overwrite did not work as expected for non-default target paths, so that may be what's happening here.

@cdanwards
Copy link
Member

@joshuayoes checked it out again and it's working with no error now. Super fast too! Good job

joshuayoes
joshuayoes commented Sep 6, 2022
View reviewed changes
Copy link
Contributor Author

@joshuayoes joshuayoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are ready for speed 🐎

src/tools/pretty.ts Show resolved Hide resolved
src/commands/new.ts Show resolved Hide resolved
src/commands/new.ts Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
src/commands/cache.ts Show resolved Hide resolved
@joshuayoes joshuayoes merged commit 8388a9a into maverick Sep 6, 2022
@joshuayoes joshuayoes deleted the feat/new-perf branch September 6, 2022 21:15
@mazenchami
Copy link
Contributor

mazenchami commented Sep 7, 2022 •
edited
Loading

@joshuayoes—"I have the need...the need for speed"

I have the need...the need for speed

@flexbox flexbox mentioned this pull request Sep 15, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamonholmgren jamonholmgren jamonholmgren approved these changes

Assignees

@joshuayoes joshuayoes

Labels
enhancement maverick Ignite v8: Maverick
Projects
None yet
Milestone
Ignite v8: Maverick
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@joshuayoes @cdanwards @mazenchami @jamonholmgren