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

lazy load unused lambda packages in core #2324

Merged
merged 4 commits into from May 27, 2019
Merged

lazy load unused lambda packages in core #2324

merged 4 commits into from May 27, 2019

Conversation

kyeotic
Copy link
Contributor

@kyeotic kyeotic commented Feb 14, 2019

NOTE: #2278 was the orignal PR, which I screwed up with a rebase. Im reopening because my git-fu is insufficent to fix it.

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests (NA)
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

This is to address loading time issues raised in #2162

There are three changes of note.

  1. In lambda-server-core I moved the subscriptions-transport-ws and apollo-engine-reporting into lazy-loaded requires and changed the top-level imports to just import the type. Since lambda's cannot take advantage of these packages, loading them is pure cost. I didn't see any other type-only imports to style after, so I followed the handbook convention of underscore-prefix-capitalized.
  2. I had the util.promisify polyfill check the node version before loading itself. NOTE: there are already tests in node 6 and node 8 that exercise this code
  3. this is a hack I added an ENV VAR check into runtimeSupportsUploads() that looks for AWS_EXECUTION_ENV, which the lambda runtime sets. This checks stop the import of apollo-upload from occuring. The library is unfortunately a top-level import, so moving it anywhere that apollo-server-lambda could have controlled directly without changing the contract was impossible. A better solution probably exists that does not hack its way through process.env. The best solution would be removing apollo-upload from the core entirely, since it surely does not belong there.

These changes combine to reduce the cold-start time of a 512mb AWS Lambda from ~450ms to ~250ms.

Before
image

After
image

(Yes, the flame graph has a rendering bug, but the numbers are accurate)

512 is a pretty beefy lambda for node, 256 is more common. These effect will be amplified the lower memory you go, because AWS matches the CPU to the allocated memory. If it is necessary, I can re-run these performance profiling on smaller lambdas.

@kyeotic kyeotic mentioned this pull request Feb 14, 2019
4 tasks
@kyeotic
Copy link
Contributor Author

kyeotic commented Feb 28, 2019

@abernix @martijnwalraven really hoping to get this looked at, its a non-breaking change with a significant performance improvement

Tim Kye and others added 2 commits March 6, 2019 18:43
hacky lambda process.env fix"

fix linting issues

update changelog

fix polyfill check

use AWS_EXECUTION_ENV

fix styling

fix version check

Update CHANGELOG.md
@pierreis
Copy link

pierreis commented Mar 20, 2019

Awesome work.

Really looking forward to see that one merged ASAP.
For now, defaulting to excluding packages in webpack, but this is far from ideal. The difference in bundle size as well as cold start time is significant.

@kyeotic
Copy link
Contributor Author

kyeotic commented Mar 20, 2019

@pierreis Maybe try adding a 👍, I'm not really sure what I need to do to get the maintainers to look at this. I've given up rebasing until one of them responds

@pierreis
Copy link

Done.
@martijnwalraven @abernix : any thoughts about the steps to get this merged?

@kyeotic
Copy link
Contributor Author

kyeotic commented May 1, 2019

@martijnwalraven @abernix Its been a few months. Is there something I can do to help move this forward?

@holmankb
Copy link

holmankb commented May 3, 2019

+1 Would be great to see this merged in.

@abernix abernix self-assigned this May 27, 2019
@abernix abernix changed the base branch from master to release-2.6.0 May 27, 2019 17:49
…motives.

The work in #2054 was
designed in a way that, irregardless of the environment, the
`graphql-upload` package would only be loaded if uploads were enabled.
Therefore, the guard which checks `process.env.AWS_EXECUTION_ENV` should no
longer be necessary.

Additionally, we don't need to prefix our type-only variables with
underscores, as that's not a style that we've otherwise adopted.
@abernix abernix added this to the Release 2.6.0 milestone May 27, 2019
@abernix
Copy link
Member

abernix commented May 27, 2019

I really appreciate you opening this PR, and it demonstrates very clearly how important it is that we always consider the dependency tree anytime we introduce any new downstream dependencies, particularly since Apollo Server runs in so many different environments, some of which are particularly sensitive to the additional cost of extensive module resolution (and other times, just because certain modules are not compatible with all environments!)

The work in this PR was mostly also implemented by #2305, #2304 and #2054, but the remaining subscriptions-transport-ws and unnecessary util.promisify imports are still super worth addressing. So, thank you!

I've made a couple tweaks, but will cut this into the next 2.6.0 alpha release!

@abernix abernix merged commit 1a4331d into apollographql:release-2.6.0 May 27, 2019
@abernix abernix mentioned this pull request Jul 16, 2019
@kyeotic kyeotic deleted the lambda-lazy branch July 16, 2019 16:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants