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: enormous memory consumption of the dev server #4148

Merged
merged 12 commits into from
Jun 9, 2023

Conversation

jayu
Copy link
Contributor

@jayu jayu commented May 22, 2023

There was not issue for that, I've decided to fix the problem right away

Context

We have a blitz v2 app with ~290 queries and mutations.

Each query/mutation indirectly imports up to 900 source code files.
We use dependency injection and as a result each endpoint imports the whole backend code.

We use next@12 and have SWC enabled.

Our dev server used to take up to 18GB or RAM, which is enormous amount of memory for parsing and merging together 900 source code files + compiling the font-end.

The memory was allocated for short period of time, and then reduced to 6-8GB.

It was killing 2020 MacBook Pro with 16BG of Ram.

image

What are the changes and their implications?

I dig into the problem for quite a while and I found out that the issue was basically using dynamic imports for importing routes code.

Dynamic imports in Webpack automatically creates a separate chunk.

Separate chunk means a separate isolated compilation of code. Chunking backend code does not make any sense unless it's run as lambda function.

With dynamic imports and the size of our app, Webpack was bundling the same code ~290 times instead of doing it once.

That created huge memory peaks in quite random moments, eg. a 30 seconds after pages/api/rpc/[[...blitz]].ts finished to compile.

To fix that I simply changed the dynamic import into require call, which do not create separate chunk in Webpack.

We are using the patched version for more than a week now and everything works fine. Compilation is faster, there are no more memory peaks and most importantly we reduced the team frustration.

Preparing a reproducible demo that showcase the memory usage might not be easy without real production code, but at least I can prove that the chunks are created for each query/mutation file when using dynamic imports. Let me know if that's needed.

Bug Checklist

  • Changeset added (run pnpm changeset in the root directory)
  • Integration test added (see test docs if needed)
    • I believe existing tests should cover this change, cos it only changes how to code is bundled without affecting runtime

@changeset-bot
Copy link

changeset-bot bot commented May 22, 2023

🦋 Changeset detected

Latest commit: ecf8fca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@blitzjs/rpc Patch
next-blitz-auth Patch
@blitzjs/next Patch
@blitzjs/auth Patch
@blitzjs/codemod Patch
@blitzjs/config Patch
@blitzjs/generator Patch
blitz Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jayu jayu changed the title fix: enormous dev server memory consumption fix: enormous memory consumption of the dev server May 22, 2023
@jayu jayu marked this pull request as ready for review May 22, 2023 19:15
@jayu jayu requested a review from flybayer as a code owner May 22, 2023 19:15
Copy link
Member

@siddhsuresh siddhsuresh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR!

Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

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

ok this is good, but resolversDynamicImport needs to default to true on vercel.

Secondly, add more info to the release notes describing why we have this, why we're changing, that the new default is different than before, and that it defaults to true on vercel.

@siddhsuresh siddhsuresh requested a review from flybayer June 9, 2023 13:40
@flybayer
Copy link
Member

flybayer commented Jun 9, 2023

I just pushed a commit with a fix and refactor.

  • resolversDynamicImport was not actually being passed to webpack
  • We had duplicate loader option types, so I removed the one, now we have proper type safety to catch the first problem
  • clean up the logic in the loader by defaulting the dynamic import config at the webpack config level

@flybayer flybayer added the 0 - <(^_^)> - merge it! ✌️ Kodiak automerge label Jun 9, 2023
@kodiakhq kodiakhq bot merged commit c7ac86b into blitz-js:main Jun 9, 2023
29 of 30 checks passed
@blitzjs-bot
Copy link
Contributor

Added @jayu contributions for doc and code

@jayu
Copy link
Contributor Author

jayu commented Jun 9, 2023

Thanks for help with shipping this 🙏

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

Successfully merging this pull request may close these issues.

4 participants