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

perf: re-order Dockerfile stages to significantly optimize build speed #58

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

kdmccormick
Copy link
Contributor

Description

Previously, build stages were ordered like this in the
rendered Dockerfile (dependencies in parenthases):

  • base
  • i18n (base)
  • app1-src (base)
  • app1-i18n (base, app1-src)
  • app1-dev (base, app1-src, app1-i18n)
  • app1 (app1-dev)
  • app2-src (base, app2-src)
  • app2-i18n (base, app2-src)
  • app2-dev (base, app2-src, app2-i18n)
  • app2 (app2-dev)
  • ...
  • production (app1, app2, ..., appN)

MFEs in dev mode only use the output of the *-dev stages.
However, because some of the *-dev stages came after
the intermediate production stages in the Dockerfile (app1, app2, etc),
Docker unnecessarily built the prod stages whenever tutor dev start
was run, which often forced users to wait through the prod-only
npm run build.

This commit reorders the build stages to look like this:

  • base
  • i18n (base)
  • app1-src (base)
  • app1-i18n (base, app1-src)
  • app1-dev (base, app1-src, app1-i18n)
  • app2-src (base, app2-src)
  • app2-i18n (base, app2-src)
  • app2-dev (base, app2-src, app2-i18n)
  • ...
  • app1 (app1-dev)
  • app2 (app2-dev)
  • ...
  • production (app1, app2, ..., appN)

Now that the *-dev stages are all before the production stages,
the MFE dev build should be significantly quicker.

Review

@arbrandes Can you take a look?

Previously, build stages were ordered like this in the
rendered Dockerfile (dependencies in paranthases):

* base
* i18n        (base)
* app1-src    (base)
* app1-i18n   (base, app1-src)
* app1-dev    (base, app1-src, app1-i18n)
* app1        (app1-dev)
* app2-src    (base, app2-src)
* app2-i18n   (base, app2-src)
* app2-dev    (base, app2-src, app2-i18n)
* app2        (app2-dev)
* ...
* production  (app1, app2, ..., appN)

MFEs in dev mode only use the output of the *-dev stages.
However, because some of the *-dev stages came after
the intermediate production stages in the Dockerfile (app1, app2, etc),
Docker unnecessarily built the prod stages whenever `tutor dev start`
was run, which often forced users to wait through the prod-only
`npm run build`.

This commit reorders the build stages to look like this:

* base
* i18n        (base)
* app1-src    (base)
* app1-i18n   (base, app1-src)
* app1-dev    (base, app1-src, app1-i18n)
* app2-src    (base, app2-src)
* app2-i18n   (base, app2-src)
* app2-dev    (base, app2-src, app2-i18n)
* ...
* app1        (app1-dev)
* app2        (app2-dev)
* ...
* production  (app1, app2, ..., appN)

Now that the *-dev stages are all before the production stages,
the MFE dev build should be significantly quicker.
@kdmccormick kdmccormick requested a review from arbrandes August 18, 2022 04:35
@kdmccormick
Copy link
Contributor Author

FYI @regisb (when you're back). I realized this issue while working on https://github.com/overhangio/2u-tutor-adoption/issues/87.

@ghassanmas
Copy link
Member

In the issue I was referring is this part of code:

RUN npm clean-install --no-audit --no-fund --registry=$NPM_REGISTRY \
&& rm -rf ~/.npm

The part of rm -rf .npm I think here probably Regis meant to optimize the image total size one its finish building. I think there is a room for imporvment here, I don't know how exactly, ideally I think it would be:

When mfe is installing its depds, it can uses .npm cahches of other mfe that are already installed, but however apps can be installed in parrallel, (if you enable docker buildkit), so probably if we can have the base base to already cahces common pacakges, then defeinilty would decrease network bandwidth usage (i.e install time),. All of this would more possible or easier to implement when we have or agree on set of depends that are consistant among the mfes, which is I opened a some PRs against nutmeg branch for few mfes.

@arbrandes
Copy link
Collaborator

@kdmccormick, will review/test ASAP. Thanks for this!

Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

The changes make sense. I also tested the build, and it works fine. Approved.

@arbrandes arbrandes merged commit 2513088 into overhangio:master Aug 23, 2022
@kdmccormick kdmccormick deleted the kdmccormick/dev-build-speed branch August 23, 2022 19:02
@regisb
Copy link
Contributor

regisb commented Aug 30, 2022

This is a great contribution, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: 🚀 Closed
Development

Successfully merging this pull request may close these issues.

4 participants