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

Candidates for performance optimization #1

Open
raymondfeng opened this issue Jan 6, 2015 · 7 comments
Open

Candidates for performance optimization #1

raymondfeng opened this issue Jan 6, 2015 · 7 comments

Comments

@raymondfeng
Copy link

@bajtos
Copy link

bajtos commented Jan 6, 2015

https://github.com/strongloop/loopback/blob/master/server/middleware/rest.js#L27
we should build the handlers once at middleware level instead of per request)

That's already happening, see L38.

https://github.com/strongloop/strong-remoting/blob/master/lib/http-context.js#L68
Remove the usage of closure, use for-loop instead

IMO it's questionable how much performance we can gain by that change. We should measure the performance of both versions and pick up the fastest one.

Remove the need to bind(this)

Definitely, we should use self instead. We should start by fixing this, and only later look into replacing forEach with for-loop.

@bajtos
Copy link

bajtos commented Jan 6, 2015

https://github.com/strongloop/loopback/blob/master/server/middleware/rest.js#L27
we should build the handlers once at middleware level instead of per request)

That's already happening, see L38.

Having said that, we are creating a new array for each request later on L61 (preHandlers.concat(restHandler)). This should be definitely optimised.

@raymondfeng
Copy link
Author

for-loop is much faster than Array.prototype.forEach, see http://jsperf.com/fast-array-foreach. I already used the same technique to optimize juggler before.

@raymondfeng
Copy link
Author

L38 doesn't help as the declaration of preHandlers at line 36 is scoped to the handler function, not the middleware factory. As a result, L38 is always evaluated to true.

@raymondfeng
Copy link
Author

We should also look into usages that prevent V8 optimization - https://github.com/petkaantonov/bluebird/wiki/Optimization-killers.

https://www.youtube.com/watch?v=NthmeLEhDDM

@bajtos
Copy link

bajtos commented Jan 6, 2015

L38 doesn't help as the declaration of preHandlers at line 36 is scoped to the handler function, not the middleware factory. As a result, L38 is always evaluated to true.

Good catch!

@raymondfeng
Copy link
Author

Another PR - expressjs/cors#35

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

No branches or pull requests

2 participants