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

Improve configurability and performance of package plugin #3924

Merged
merged 7 commits into from Aug 23, 2017

Conversation

@medikoo
Copy link
Member

commented Jul 10, 2017

Firstly, sorry for not opening an issue first, it came out as natural follow-up to reducer and transpiler plugins I developed recently.

Still, let me know if I'm on a right path, maybe there's a better way to approach it.

What did you implement:

I secluded logic that's responsible for:

Having that not mixed into other methods as it's now, allows to tweak/replace functionalities in question in a more reliable way, e.g. it will allow to replace internal logic of reducer and transpiler with something more solid.

Additional smaller changes that came out naturally over the process:

More reasoning notes

Current lambda packaging process, as implemented in master, may come out as highly inefficient for some service cases:

  • Even in individual mode, lambda specific packages cover whole service codebase
  • Invoked internally glob and npm commands are slow when comparing to more direct file resolution methods, and in certain scenarios may produce infinite stalls

In SLS project I work with (which consists of about 30 lambdas), each lambda package resulted with 6MB file, where on average lambda specific code covers 200KB.

Initially on our side, packaging of single lambda took over 1 minute, it was due to specific configuration where we had some internal versioned packages (folders with package.json outside of node_modules). It appeared that main issue lies in excludeNodeDevDependencies function, which does expensive globby call, and expensive npm ls call.

What's interesting in case someone had in setup some packages linked (via npm link) then deployment script totally hanged on that (most likely because following symlinks got globby into infinite recursion).

After removing internal packages it got cut to 25s per package, and I'd say it's still really bad file paths resolution time for single lambda.

I further cut it by taking in reducer plugin, which by following require paths, resolves just needed files. It takes now 2s, but still may be improved if I would cut off the glob step totally, but for that it would be better to have improvements I proposed with this PR.

I believe that it may be a good step for SLS, to provide such lambda files resolver (for Node.js case) out of a box, as an option. If you also feel it's a good idea, I'll be happy to provide it via other PR.

How did you implement it:

I secluded mentioned logic in least invasive way and ensured that existing methods work as they did before.

How can we verify it:

Run tests

Todos:

  • Write tests
  • Fix linting errors
  • Make sure code coverage hasn't dropped (there's a very minor drop, but only due to an extra error handling improvement, that can be a result of some very rare race condition or disk I/O error, therefore problematic to write test for)
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens requested review from eahefnawy and pmuens Jul 11, 2017
@pmuens pmuens added the pr/in-review label Jul 11, 2017
@medikoo

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

@eahefnawy @pmuens what is the status of this PR exactly, is there a chance it'll be reviewed in next days? Great thanks!

@pmuens

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

@eahefnawy @pmuens what is the status of this PR exactly, is there a chance it'll be reviewed in next days? Great thanks!

Thanks for taking time to implement this change and for pinging @medikoo 👍!

We have this PR on our radar and will review it in the upcoming days!

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2017

I ensured it's up to date with master. It'll be great to have that considered, as it'll significantly boost lambda packaging performance on our side.

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Aug 22, 2017

@medikoo Sorry for the delay. I've reviewed and took this PR for a spin on a fresh service and found no issues. However I wasn't able to verify that it actually improves the performance.

Got any example service (maybe with some npm packages) with specific config that could illustrate how this PR improves performance? or maybe some benchmarks from your service? Otherwise this PR is G2M 😊

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2017

@eahefnawy It depends on a setup, but pursuing async parallel calls instead of consecutive sync calls, seems more right way to configure workflow within Node.js programs. It may bring noticeable performance difference for some extreme cases, still it's true that difference may be negligible for all others :)

However, it's not the reason I proposed those changes. What's more important in case of project I work with, is that those changes will allow to improve configuration and performance of following plugins, we rely on:

  • https://github.com/medikoo/serverless-plugin-reducer - Here I will be able to directly resolve files for zip archive, and not be forced to rely on slow glob calls (as it's the case now). This noticeably improves performance of packaging in our serverless project (lambdas will be packaged near instantly, it's not the case now).
  • https://github.com/medikoo/serverless-plugin-transpiler - Here I will be able to decorate just logic responsible for retrieval of file content, and not rewrite whole zip which takes place now, that doesn't seem future-proof.
@eahefnawy

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

@medikoo you're totally right about async/sync. It's a needed refactoring.

I'll merge this in 😊

Thanks again @medikoo

@eahefnawy eahefnawy added this to the 1.21 milestone Aug 23, 2017
@eahefnawy eahefnawy merged commit ed3c7c3 into serverless:master Aug 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

@eahefnawy Thank you!

Copy link
Member

left a comment

Thanks for the PR @medikoo 👍

I also gave this a spin today but unfortunately it seems like this fails for large packages.

I added the following package.json file in the services root directory and ran npm install. After that I ran serverless package which throws an error (ENFILE: file table overflow, open XYZ.js).

{
  "name": "foobar",
  "dependencies": {
    "etag": "^1.8.0",
    "serverless": "^1.20.2"
  },
  "devDependencies": {
    "express": "^4.15.4"
  }
}

IMHO we should compile an example service / serverless.yml file with exclude, include and other features which touches the package plugin so that we can make sure that nothing breaks (we could translate this service into an integration test later on).

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

throws an error ENFILE: file table overflow, open XYZ.js

It's wide known issue, that may happen when large number of concurrent fs calls gets in. This can be fixed by using e.g. graceful-fs. Generally it's good to have it in scripts that operate on indefinite number fs resources

I can propose a PR that loads graceful-fs it in bin/serverless. let me know

@HyperBrain

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

Regarding graceful-fs, I think it is important to use V4 (which does not monkey-patch fs by default) and do not activate the patch functionality as this affects the whole serverless process. Patching it process-wide will have side-effects on all plugins AND all user code run with invoke local as it runs within the Serverless process.

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

Regarding graceful-fs, I think it is important to use V4

Yes, this one doesn't patch. Still it's not as easy. I personally prefer to rely on patching in projects I work with, as then I know problem is solved across all dependencies I work with, and it's really seamless.

While non-patching solution requires that every entity that may open descriptors internally uses graceful-fs instead of fs which can hardly be the case.

However maybe in that single case of lambda packaging, this could indeed be fixed on spot with graceful-fs v4. I'll look into it shortly

@HyperBrain

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

The problem with invoke local is, that a patched fs silently hides issue in the user code (where it would fail without patching in a version deployed to AWS). The same is true for plugin development.

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

The problem with invoke local is, that a patched fs silently hides issue in the user code

Assuming we see an EMFILE as an issue in user code. To me it's more a node.js design issue, that can be transparently fixed by monkey patching.

It's also important to note, that issue is not hidden, it's fixed (error is just workaround by given call being postponed, not aborted, which is seamless when we speak of async calls) :)

But I agree if monkey patching can be avoided here, it's way better to avoid it.

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

@pmuens @HyperBrain I've published PR at #4145

There were two obvious hot places for fs concurrency, and after applying graceful-fs there should be no more EMFILE/ENFILE issues, @pmuens please confim it fixes problem on your side.

@pmuens

This comment has been minimized.

Copy link
Member

commented Aug 24, 2017

Thanks for PRing that @medikoo 👍. Great to see the fix that fast 💯!

I'll take a deep dive into this today!

@eahefnawy eahefnawy referenced this pull request Aug 30, 2017
@aterreno

This comment has been minimized.

Copy link

commented Sep 13, 2017

We started to notice few random loading issues with v1.21.

And we currently ended up going back to 1.20.

I wonder if anyone else is having issues and if this is a regression from this change.

All I can see on our cloudwatch logs is Unable to import module ‘handler’: Error at Function.Module._load

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

All I can see on our cloudwatch logs is Unable to import module ‘handler’: Error at Function.Module._load

@aterreno can you prepare some (as small as possible) reproducible test case, so we can look into it?

Technically changes included in this PR shouldn't alter the outcome in anyway

@pmuens

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

Thanks for your comment @aterreno and thanks for jumping in @medikoo 👍

I'm pretty sure that this is the bug where binary files were not packaged correctly. We've fixed that in v1.21.1 so updating should resolve the issue. Furthermore we've just release Serverless v1.22 a few hours ago.

@aterreno let us know if you still face the same issues after updating!

@aterreno

This comment has been minimized.

Copy link

commented Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.