-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
@abernix @martijnwalraven really hoping to get this looked at, its a non-breaking change with a significant performance improvement |
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
Awesome work. Really looking forward to see that one merged ASAP. |
@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 |
Done. |
@martijnwalraven @abernix Its been a few months. Is there something I can do to help move this forward? |
+1 Would be great to see this merged in. |
…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.
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 I've made a couple tweaks, but will cut this into the next 2.6.0 alpha release! |
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:
This is to address loading time issues raised in #2162
There are three changes of note.
lambda-server-core
I moved thesubscriptions-transport-ws
andapollo-engine-reporting
into lazy-loadedrequires
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.util.promisify
polyfill check the node version before loading itself. NOTE: there are already tests in node 6 and node 8 that exercise this coderuntimeSupportsUploads()
that looks forAWS_EXECUTION_ENV
, which the lambda runtime sets. This checks stop the import ofapollo-upload
from occuring. The library is unfortunately a top-level import, so moving it anywhere thatapollo-server-lambda
could have controlled directly without changing the contract was impossible. A better solution probably exists that does not hack its way throughprocess.env
. The best solution would be removingapollo-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
After
(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.