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

Perfomance issue #78

Closed
NPCRUS opened this issue Jan 11, 2018 · 14 comments
Closed

Perfomance issue #78

NPCRUS opened this issue Jan 11, 2018 · 14 comments

Comments

@NPCRUS
Copy link
Contributor

NPCRUS commented Jan 11, 2018

I found, that such code:
const { doNotWaitForEmptyEventLoop } = require('middy/middlewares')
or actually importing any middleware, and any amount of it adds aditional 110 +-5 ms to code execution

My lambda without adding any middlewares running approximately 35-40 seconds. And if I want to add some middleware from you library, it becomes 110 ms seconds more. In case of high availability microservices it's not really good.

BTW middy itself without attaching any existed in library middlewares works fine.

Do you have any idea, how to solve this?

Best regards

P.S: the way you can test it:

console.time('test')

const middy = require('middy')
//const { doNotWaitForEmptyEventLoop } = require('middy/middlewares')
const test = require('tape')
const utils = require('aws-lambda-test-utils')
const mockContextCreator = utils.mockContextCreator;

let handler = function(event, context) {
    context.callbackWaitsForEmptyEventLoop = false
    context.succeed("succeed")
}

handler = middy(handler)
//    .use(doNotWaitForEmptyEventLoop())

test('LambdaTest', function(t){
    t.test("test", function(st) {
      function test(result){
        console.log(result)
        st.end()
        console.timeEnd('test')
        process.exit()
      };
      const context = mockContextCreator({}, test); // no options and test as the callback
      handler({}, context);
    });
    t.end();
  });
@NPCRUS
Copy link
Contributor Author

NPCRUS commented Jan 13, 2018

I've done some research.

Looks like validator(especially ajv library) middleware loads about 140ms, which may be deadly amount of time for some services. And anyway since amount of prebuild middlewares is growing i think that it's not a good a good approach to load all of them. What I think will be possible to do here is get middlewares in such way

const validator = require('middy/middlewares').validator()

This will save execution time by not loading any other middlewares and still pretty handy and comfortable way to get middleware.

What do you think?
@lmammino

@DavidWells
Copy link
Contributor

I was thinking about this the other day as well.

We want to include only the code required for a given lambda to run.

We can optimize in two places

  1. at runtime like @NPCRUS suggests
  2. the actual project size in node_modules. The size of zip effects perf as well, so even if you only include 1 middleware, the s3 zip still needs to load and keeping things lean is a "best practice" as it were

You can solve this in userland with webpack/rollup but I think we can also help users not shoot themselves in the foot forcing this with middleware published as separate packages

We could have middleware published as separate packages with a tool like https://lernajs.io/ (or the new packaging tool the lerna creator is close to finishing)

So a user would install the lean core and only the middleware they need.

npm install middy # lean core
npm install middy-validator 
npm install middy-json-body-parser
etc 

This would keep the zip as tiny as possible.

The repo can stay a monorepo, it's just a change to how the middleware would get published to npm via lerna

@vladholubiev
Copy link
Contributor

Agree with @DavidWells 👍

This is a perfect case for setup much like Babel has. One monorepo and complementary packages under @babel npm org scope.

@lmammino
Copy link
Member

lmammino commented Jan 14, 2018

Very good points from @NPCRUS and @DavidWells. The current architecture (and examples) are definitely helpful in terms of performance when using the bundled middlewares.

I like the options of using a mono-repo approach and separate the middlewares from the main core, so this might be something to consider for the near future, maybe targeting the first stable 1.0.0 release.

At the moment there's a very easy fix anyway, you can import middlewares directly as follows:

// imports only one middleware (doNotWaitForEmptyEventLoop)
const doNotWaitForEmptyEventLoop = require('middy/src/middlewares/doNotWaitForEmptyEventLoop')

This should do, until we figure out how to switch to a mono-repo approach.

Comments/thoughts?

@lmammino
Copy link
Member

Also, @vladgolubev, I really like the idea of having scoped package as babel. we might have the following approach:

const middy = require('@middy/middy') // or '@middy/core' or simply 'middy'
const doNotWaitForEmptyEventLoop = require('@middy/doNotWaitForEmptyEventLoop')

@kevinrambaud
Copy link
Contributor

Hey @lmammino any update about the approach you guys would prefer ?

@lmammino
Copy link
Member

lmammino commented Feb 3, 2018

Hello @kevinrambaud 😉
For the moment I would suggest to use the approach mentioned above to avoid to load all the middlewares (and their dependencies) at once:

const doNotWaitForEmptyEventLoop = require('middy/src/middlewares/doNotWaitForEmptyEventLoop')

Moving towards a more stable 1.0.0 (yet to be planned), we will probably move towards a mono-repo approach where no middleware is bundled with the core and you have to install/require each and every one of them separately (similar to express or fastify).

@kevinrambaud
Copy link
Contributor

if you need any help, I would highly be interested to contribute 😁

@lmammino
Copy link
Member

lmammino commented Feb 3, 2018

Definitely, all help is appreciated. I'll try to lay out a roadmap for 1.0.0, meanwhile, being an open source project, every area you feel needs an improvement, is probably something that deserves a PR or a discussion, so feel free to make your own suggestions 😉

@karlbateman
Copy link

It'd be prudent to update your documentation to include this workaround.

@lmammino
Copy link
Member

Hello @karlbateman, would you like to submit a PR for that?

@karlbateman
Copy link

@lmammino Will do 👍

@lmammino
Copy link
Member

Awesome, i look forward to seeing your PR :)

@willfarrell
Copy link
Member

Closing, v1-beta is now out with mono-repo support.

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

No branches or pull requests

7 participants