Create a gist now

Instantly share code, notes, and snippets.

What would you like to do?
Response to Netflix's "Node.js in Flames" Blog Post

If you haven't read Netflix's Node.js in Flames blog post you should. It is a great deep dive into debugging a node performance problem. The post includes useful tips that can help you solve similar problems.

That said...

My feedback from the perspective of a framework developer is quite different. I found the tone and attitude towards express.js to be concerning and somewhat offensive. Here was a developer blaming the framework he chose for poor architecture when they never bothered to actually learn how the framework works in the first place. Everything that followed originated from this basic lack of understanding.

Express uses a very simple router logic which is at the core of how express works, so let’s examine that first (my knowledge of express is somewhat dated but I think the principle is still the same). Express keeps a hash of the various HTTP methods (GET, POST, etc.) and for each method, an array of middlewares. Each middleware is just a function with signature function(req, res, next) and an regular expression used to decide when to execute the middleware based on the incoming request path.

The router logic is pretty simple. When a request comes in, the routing table it looked up using the HTTP method which returns an array or middlewares. The array is then iterated over by matching the request path to the regular expression assigned to the middleware. When you add generic middleware to express (that is, not path-specific), those are added to the same array but without any path requirements. When a middleware is invoked, it can end the workflow by not calling the next() callback (or call it with an error). If next() is called, the next middleware match is executed. This is how all your app.use() middlewares are called before and after the route handler.

While this is not a design pattern I would choose, it is simple and elegant. It keeps the entire framework codebase minimal and consistent with its architecture. It is really all about middleware, some with filters (e.g. path handlers).

The post criticizes express for storing the routing table in an array vs a hash or tree. As it turns out, the complexity tradeoff between iterating over an array (which tends to be short given most application routing requirements) and walking a tree makes arrays a much better choice. I’ll also point out that Restify, the alternative framework Netflix listed as their new choice, does the same thing (though instead of recursive calls, it uses a for loop). The only router I know that uses a fancy tree is director and that design significantly handicapped it’s feature set and usefulness.

Matching routes to requests is tricky because developers like to use everything for defining their routes. This includes simple strings, regular expressions, wildcards, and path parameters. You can’t store these in a hash because you cannot look up a regular expression match based on a string. You have to iterate somewhere. A hash will only work if you limit routes to static string values (and if that’s the case, why use a router at all).

The criticism about allowing routes to repeat in the express array shows that even after doing all this work, the Netflix team still doesn’t understand how express works. I am only pointing this out because they made a public statement putting express down without acknowledging the reasons. The middleware architecture requires repeating the same path multiple times in the array because that’s how the matching works. It also allows powerful chaining of small actions on a route without having to collect them all in a single function (e.g. pre and post route processors).

The express design, and for that matter all the other framework I am familiar with except my own allows adding conflicting routes. This is not a bug but an outcome of their extremely flexible route matching support allowing regular expressions and wildcards. You cannot compare two regular expressions to decide if and how much they overlap, and in which order they should be compared. This is a limitation coming directly from the feature set. It is a simple tradeoff.

In hapi, we limit the types of routes you can add so that we can enforce strict limits on conflicts. We also worked hard to ensure that the order in which routes are added doesn’t matter. Routes are matched based on a deterministic algorithm that sorts the table on every addition. This was a very important feature for us working in a large team where people might not be aware of the routes added by others, or where the order in which plugins are loaded can cause unexpected production issues. These are all decisions we made based on months of hands-on experience building applications on top of express and director.

The Netflix post does take responsibility for failing to understand how the framework they chose works. But that admission does not excuse criticizing the framework as inept. The express architecture has worked well for many people. It has known limitations which is why there are so many other frameworks to choose from. One isn’t superior to the other without the context of your use case and requirements. There is no “best”.

rexxars commented Nov 19, 2014

I mostly agree with you - the post came across as "express is a bad choice", for the most part. However, they did, to some extent, admit it was their own fault for not fully understanding their dependencies and misusing the express API:

What did we learn from this harrowing experience? First, we need to fully understand our dependencies before putting them into production. We made incorrect assumptions about the Express.js API without digging further into its code base. As a result, our misuse of the Express.js API was the ultimate root cause of our performance issue.

Restify is misspelled in this text (it was written as Resity, missing a f)

Owner

hueniverse commented Nov 19, 2014

@rafaelverger thanks.

crertel commented Nov 19, 2014

I didn't really view the post as "Express is a bad framework" so much as "This piece of Express is not coded to be as performant as it could be". I think people may be reading too much into that.

Marak commented Nov 19, 2014

I read:

This turned out be caused by a periodic (10/hour) function in our code. The main purpose of this was to refresh our route handlers from an external source. This was implemented by deleting old handlers and adding new ones to the array.

refresh our route handlers from an external source

This is not something that should be done in live process. If you are updating the state of the node, you should be creating a new node and killing the old one.

Aside from hitting a somewhat obvious behavior for fucking with the state of express in running process, once you have introduced the idea of programmatically putting state into your running node you have seriously impeded the abiltity to create a stateless fault tolerant distributed system.

@crertel That's missing the point too. Micro-optimizations aside, it is coded as performant as it can be (as Eran explains, you can't use a map), which demonstrates that the Netflix devs completely misunderstood the express routing system (it purposely allows for multiple routes with the same name/regex/wildcard to enable composability and modularization). It's well documented, and a cursory google search for 'express.js middleware' would show how it works.

Attaching a click-bait title to a blog post where the dev tries to blame the tools for their own mistakes is the main problem Eran is addressing.

None of this means that the Router in express can't be improved performance-wise though. Building things into an alternating regexp, or a tree of some sort, where possible, is a potential solution to some systems routing performance problems. It might be non-trivial to make it work right, but it is possible. Sure it's more complex, but if Express is serving up billions (trillions?) of page requests a day across the world (counting every system using express), it is worth it.

Of course talk is cheap, so I'll shut up and do some experiments :)

@huniverse Thanks.

I posted on their facebook comment section; it would have been nice if they had of just come to us with the issue.

In addition to the global array thing, under situations where you have too many routes, nesting apps (routers) is a good option. Mounting express apps to express apps is indeed a thing. However they seem to have completely ignored the possibility of such..

As for tree-routers, jonathan made this recently-ish: https://github.com/pillarjs/routington - and we will hopefully be integrating it into an express-compatible router in the future.

I do however admit our documentation could be better. These are things we are working on. It would be nice if companies that used express also supported it. :/

Maybe the post has been edited since your response, but I don't see where it calls (or even suggests) Express is inept...

In the conclusion it seems to me they accept all the blame: "our misuse of the Express.js API was the ultimate root cause of our performance issue"

@Marak

This is not something that should be done in live process. If you are updating the state of the node, you should be creating a new node and killing the old one.
Aside from hitting a somewhat obvious behavior for fucking with the state of express in running process, once you have introduced the idea of programmatically putting state into your running node you have seriously impeded the abiltity to create a stateless fault tolerant distributed system.

Thank you, that's exactly what I thought when first reading the post. In my opinion this design decision asks for trouble, one way or another. In any framework.

@hueniverse thanks for a great post.

Restify is misspelled in this text (it was written as Resitfy, with the 't' and 'i' transposed)

orclev commented Nov 19, 2014

Yeah, I don't get where you're getting that they were particularly negative to express. They readily admit that it was all their fault and that they were using the express API incorrectly. They do question why express went with a linear array of routes instead of some kind of map, but I don't feel like they were particularly judgemental in making that statement, it's a valid question. The answer to that question is of course because they went with a set of regex to match the routes on, but there are of course other increasingly complex ways of achieving the same thing that could have utilized either a map or a tree for dispatch, it's merely a question if whether the savings on request dispatch are worth the extra overhead (both cognitive, and procedural) involved in using those methods.

Ultimately I feel like it was a very good write up, I thought it was a fascinating post mortem of a performance problem they were seeing. That said, ignoring the complaints about negative tone, I thought this gist was also a nice breakdown of why express performs routing the way it does.

Fully agree with orclev, I did not sense any negativity towards the Express framework from the Netflix article and thought it was an insightful technical read.

@ericdgreene

Here are the parts where they heavily suggest it is express's fault (emphasis mine):

  • Route handlers for all endpoints are stored in one global array.

A global array is not the ideal data structure for this use case. It’s unclear why Express.js chose not to use a constant time data structure like a map to store its handlers. Each request requires an expensive O(n) look up in the route array in order to find its route handler. Compounding matters, the array is traversed recursively. This explains why we saw such tall stacks in the flame graphs. Interestingly, Express.js even allows you to set many identical route handlers for a route. You can unwittingly set a request chain like so

[...]

having needless spent time spinning through a, b and multiple instances of c.

[...]

It would have been nice for Express.js to throw an error whenever there’s more than one route handler chain for a route.

[...]

Unfortunately, it was also inadvertently adding a static route handler with the same path each time it ran. Since Express.js allows for multiple route handlers given identical paths, these duplicate handlers were all added to the array. Making matter worse, they were added before the rest of the API handlers, which meant they all had to be invoked before we can service any requests to our service.

Even knowing that it was their own code and misunderstanding of how express works (ie; not reading the docs / learning from examples prior to use in production), they still negatively point to features of express which are by design and blame them for the issues.

It's all about the negative tone when referring to express.js that Eran is pointing out.

Twipped commented Nov 19, 2014

It's more the tone and structure of the article, rather than the specific things they say.

A global array is not the ideal data structure for this use case. It’s unclear why Express.js chose not to use a constant time data structure like a map to store its handlers. Each request requires an expensive O(n) look up in the route array in order to find its route handler. Compounding matters, the array is traversed recursively. This explains why we saw such tall stacks in the flame graphs. Interestingly, Express.js even allows you to set many identical route handlers for a route. You can unwittingly set a request chain like so.
[...]
It would have been nice for Express.js to throw an error whenever there’s more than one route handler chain for a route.
[...]
Since Express.js allows for multiple route handlers given identical paths, these duplicate handlers were all added to the array.

It isn't until the end of the article that they reveal it was all their fault, so anyone just skimming the text can quickly conclude that this is a bug in Express (and indeed, all three posts on Reddit for the article have comments from people who thought that was the point of the post).

The emphasis of the text is not "Look how we used flame-graphs to solve this problem"; It is "Look at this bug that express let us put into our software."

I agree that the Netflix post title and part about the repeated routing could have been rethought, but I also did not walk away from reading the article thinking that the writer was blaming Express for being inept. I thought it clearly stated that it was their fault, and they were taking actions to understand why it was their fault, and how to protect from that happening again. Otherwise, I was actually quite impressed by the overview of the methods taken to find the problem. This is ultimately what I think that post was about.

It's subtle and may not be what you meant, but your post comes across to me as also slighting Express to some extent (using a pattern you wouldn't choose, hapi avoids these issues based on experience with Express, other frameworks to choose from, ...), and plugging hapi at the same time. If this post was meant to truly be a response to Netflix as per your title, then I really don't think that any of the hapi bit was necessary or supportive of your point. It has nothing to do with the Netflix post or Express, other than to indicate that hapi is an improvement over Express, which could also be read as a public statement that is putting express down.

Good job on the rest of your post however, especially the considerations and difficulties involving route matching, composability, modularity, etc.

yunong commented Nov 19, 2014

Eran - thanks for reading the article and for taking the time to provide
thoughtful feedback. One of the things we highly value at Netflix is
transparency and candidness. We thought that our experience in using flame
graphs to troubleshoot a performance issue was an interesting story to share.

We certainly could have told this story while glossing over some of the
details. However, we felt it was better to be completely transparent on what we
uncovered in our investigation, as well as take responsibility for the root
cause. Any other observations we made along the way are included in the post,
and it is up to the reader to decide how much they want to focus on or weigh
any of those observations. It is absolutely not our intent to blame the
framework, but to instead cast a critical eye towards all of the factors at
play in this particular instance.

The broader takeaway I'd like for folks to have from this post is the perf
troubleshooting approach and techniques we used, as well as the cautionary tale
to take time to really understand how your dependencies work.

I'd like clarify some of the points of my post.

The specific problem we encountered was not a global handler but the express
static file handler with a simple string path. We were adding the same static
router handler each time we refreshed our routes. since this route handler was
in the global routing array, it meant that every request that was serviced by
our app had to iterate though this handler.

It was absolutely our mis-use of the Express API that caused this -- after all,
we were leaking that specific handler! However, had Express 1) not stored
static handlers with simple strings in the global routing array, and 2)
rejected duplicate routing handlers, or 3) not taken 1ms of CPU time to merely
iterate through this static handler, then we would not have experienced such
drastic performance problems. Express would have masked the fact that we had
this leak -- and perhaps this would have bit us down the road in another subtle
way.

Express uses a very simple router logic which is at the core of how express
works, so let’s examine that first (my knowledge of express is somewhat dated
but I think the principal is still the same). Express keeps a hash of the
various HTTP methods (GET, POST, etc.) and for each method, an array of
middlewares. Each middleware is just a function with signature function(req,
res, next) and an regular expression used to decide when to execute the
middleware based on the incoming request path.

Our application has over 100 GET routes (and growing), even using the Express's
Router feature -- which lets you compose arrays of handlers for each path
inside the global route array, we'd still have to iterate through all 100
handlers for each request. Instead, we built our own custom global route
handler, which takes in the context of a request (including its path) and
returns a set of handlers specific to the request such that we don't have to
iterate through handlers we don't need.

This was our implementation, which separated the global handlers
that every request needs from handlers specific to each request. I'm sure
more optimal solutions are out there.

The criticism about allowing routes to repeat in the express array shows that
even after doing all this work, the Netflix team still doesn’t understand how
express works. I am only pointing this out because they made a public statement
putting express down without acknowledging the reasons. The middleware
architecture requires repeating the same path multiple times in the array
because that’s how the matching works. It also allows powerful chaining of
small actions on a route without having to collect them all in a single
function (e.g. pre and post route processors).

While this approach is probably good enough for smaller applications --
computers are getting fast, after all -- at larger scales it worth optimizing
this approach. We envision having 100s or even 1000s of routes in our
application in the future, having more efficient approaches would be helpful.
This applies to all frameworks, not just Express.

Finally, regarding our transition to Restify -- our main motivation for was for
its observability. It includes tools such as
DTrace(http://mcavage.me/node-restify/#DTrace) probes which would have made
solving our problems a trivial one.

orclev commented Nov 19, 2014

@jesstelford
Most of those are valid statements and I don't find them particularly negative. There are reasons express works the way it does, but they're correct in pointing out these gotchas that you can run into as a user of express. If anything I think the take away is that express probably needs to review the way they document the route handling API to make it clearer the way it functions and to more explicitly warn about these issues without requiring you to dig into the source code to understand it.

It's also unfortunate that express allows you to register multiple identical handlers for the same route, although it isn't entirely clear there's a good way of preventing that without breaking other features of express. At the very least some kind of warning might be nice if you have multiple handlers with the same (exactly the same) route which should be relatively simple to do. The more complex case of routes that have overlapping regex of course is probably too complex to handle.

They are critical of some pieces of express, but I don't think they are unduly so, as they're the places where express deviates from the expectation that a casual reading of their API would give you. It isn't that express is wrong, as Netflix points out, the onus was on them to understand the API before they went into production with it, but it does serve as a useful warning to other users of express for potential issues they might run into, and a good set of recommendations for the authors of express for sections of the API that probably need a bit better documentation.

@orclev

they're correct in pointing out these gotchas

The problem is, they didn't point them out as gotchas. Instead, they said negative things about the implementation without giving a workaround (a 'gotcha' in my eyes is "Here's a potential pitfall, here's how to avoid it.").

express probably needs to review the way they document the route handling API

Agreed - more documentation is definitely a good thing!

It's also unfortunate that express allows you to register multiple identical handlers

The thing is, that's done by design. And for good reason! Here's a (slightly contrived) example:

// handlers/logged-in.js
module.exports = function(app) {
  app.get('/account/*', function (req, res, next) {
    var loggedIn = /* Some logic to ensure user is logged in */
    loggedIn ? next() : /* redirect to /login */
  });
}
// handlers/account-menu.js
module.exports = function(app) {
  app.get('/account/*', function (req, res, next) {
    /* Some logic to display the My Account pull-down menu */
    next();
  });
}
var express = require('express');
var requireLogin = require('./handlers/logged-in');
var showAccountMenu = require('./handlers/account-menu');
var app = express();
requireLogin(app);
showAccountMenu(app);
app.listen(3000);

Using this kind of modularized code structure, you can reason that the requireLogin route will be matched first, and the following routes only executed if the user is logged in. This approach means you no longer need to check if (loggedIn) in each of the following route handlers, keeping your code nice and clean.

edit Whoops, got my logic back to front in the code example. Fixed now.

[Express collaborator here]

However, had Express 1) not stored
static handlers with simple strings in the global routing array, and 2)
rejected duplicate routing handlers, or 3) not taken 1ms of CPU time to merely
iterate through this static handler, then we would not have experienced such
drastic performance problems. Express would have masked the fact that we had
this leak -- and perhaps this would have bit us down the road in another subtle
way.

Going to repost what I posted earlier:

I posted on their facebook comment section; it would have been nice if they had of just come to us with the issue.

[...]

I do however admit our documentation could be better. These are things we are working on. It would be nice if companies that used express also supported it. :/

@orclev

It's also unfortunate that express allows you to register multiple identical handlers for the same route, although it isn't entirely clear there's a good way of preventing that without breaking other features of express. At the very least some kind of warning might be nice if you have multiple handlers with the same (exactly the same) route which should be relatively simple to do. The more complex case of routes that have overlapping regex of course is probably too complex to handle.

A warning may be due.

and a good set of recommendations for the authors of express for sections of the API that probably need a bit better documentation.

Indeed.

@jesstelford

The thing is, that's done by design. And for good reason! Here's a (slightly contrived) example:

That being said, I feel like it would be better to have these as a .use middleware. I'd have to check the internals but I do think that warning for handlers [the http methods] may be possible for duplicates; in which case it would make sense, since they are typically only used as endpoints.

@Fishrock123

I feel like it would be better to have these as a .use middleware.

Which furthers to emphasise the point that express.js is very powerful even though it has a fairly straight-forward minimal routing implementation :)

edit: To show an example with use:

// handlers/logged-in.js
module.exports = function (req, res, next) {
  var loggedIn = /* Some logic to ensure user is logged in */
  loggedIn ? next() : /* redirect to /login */
}
// handlers/account-menu.js
module.exports = function (req, res, next) {
  /* Some logic to display the My Account pull-down menu */
  next();
}
var express = require('express');
var requireLogin = require('./handlers/logged-in');
var showAccountMenu = require('./handlers/account-menu');
var app = express();

app.use('/account/*', requireLogin);
app.use('/account/*', showAccountMenu);

app.listen(3000);

@jesstelford

Which furthers to emphasise the point that express.js is very powerful even though it has a fairly straight-forward minimal routing implementation :)

To be fair, the fact that there's debate here around whether this should be used as .use middleware or not could be viewed as a bad thing (i.e. the framework is too flexible, or not structured enough).

In either case, the naive view of semantics around app.use are quite different to the semantics around app.get. It's not unreasonable to expect that a user would expect app.get routes to be de-duplicated, where app.use middlewares, for obvious reasons, would not. It's all fine to say "RTFM", but really, when APIs appear to be simple enough to understand without reading the manual, but have side effects or unforeseen circumstances, perhaps they need to be named a bit better or changed around a bit.

@spronkey

All good points! I'm all for self-documenting APIs and code. I know the express team (and most open source node.js projects) are open to Pull Requests with good suggestions. It might be worthwhile proposing a solution (such as @baudehlo has above) to the devs. Personally, I think I'm too close to the trees to see the forest - I don't have any problems understanding the way HTTP methods vs use work, so I'm probably not the best to suggest improvements in that regard.

Marak commented Nov 20, 2014

Hey guys.

I use Netflix all the time.

Please don't fuck it up so it won't work anymore.

We are 100% open to any PR or issue. There has not yet been a single one spurred from the Netflix article, though, and I am not all-knowing and just am magically aware of something because there was a blog post about it--make an issue over in GitHub is there is an issue with Express. Make a PR over in GitHub if there are changes with Express.

There is even a branch that is 27 days old that removes the recursive lookup for a linear loop (so it removes call stack creation overhead): https://github.com/strongloop/express/tree/iterative-router . I also released that change as npm install router@1.0.0-beta.2 as part of the splitting of Express.

Twipped commented Nov 20, 2014

@yunong

However, had Express 1) not stored static handlers with simple strings in the global routing array...

... then it wouldn't be in the request chain at all and could never be invoked. Static handlers are middleware just like everything else. They have to go into the middleware stack. There's also nothing that says that they have to be the first thing in the stack. You can put them anywhere as long as another route loaded before them doesn't match the static asset's url.

even using the Express's Router feature -- which lets you compose arrays of handlers for each path
inside the global route array, we'd still have to iterate through all 100 handlers for each request.

Incorrect, not when you use it properly. Say you've got 100 routes, evenly spread across ten base paths.

  • /a/a
  • /a/b
  • /a/...
  • /b/a
  • /b/c
  • etc...

Organize the routes under /a into one router that gets mounted as .use('/a', routerA), and all routes for /b into another router that gets mounted as .use('/b', routerB), and all for /c, etc etc.

A request to /d/c will iterate like so:

  1. /a
  2. /b
  3. /c
  4. /d -> Invoke Router
    1. /d/a
    2. /d/b
    3. /d/c -> Invoke Handler

The router trees are only descended if the base path matches. So assuming an even distribution (and yes, I know there's no chance your app is evenly distributed), even the very last route of the very last router will only be 20 iterations. Never 100.

@spronkey:

the framework is too flexible, or not structured enough

Providing structure is not Express' job. It is not that kind of framework. Express' flexibility in this matter is why it is able to serve as the core in so many other structured frameworks like Sails and Kracken.

I also want to comment about people mentioning "Express.js vs Hapi", probably only because the Hapi creator wrote this post :) Express.js and Hapi coexist and they approach the same problem from two completely different sides and they each have their merits. One may be good for some cases, the other for other cases.

I'm sure there's no valid reason to have several web frameworks, just like we don't have different programming languages, operating systems, etc., all with their pros and cons. The user needs to decide what suites their needs.

tj commented Nov 20, 2014

"We'd still have to iterate through all 100 handlers for each request" - V8 is very good at this sort of thing so I wouldn't worry about that until it shows up in a profile. V8's regexps are are compiled to machine code as well so they're very fast, I've never found it to be a bottleneck in any application I've worked on, if it was we would have improved it.

It does seem like a documentation issue, or at very least some bounds checking to warn, similar to Emitter.

simoami commented Nov 20, 2014

@hueniverse by principal, I think you meant principle

Some great discussions. While reading Netfilx's post, I was thinking "Oh cool, they found an issue - hope they contribute back to the open-source community...wait...nope...not even contributing to the docs & instead changing frameworks?! Well done Netflix."

simoami commented Nov 20, 2014

Let's not forget that Netflix have their own web architecture / open source platform, mostly powered by java. The fact that they use Node.js is a big win for Node.js community.

Totally agree. The engineer who wrote the article was completely offbase, and it came off offensive to me as well. Netflix should be ashamed.

fadzlan commented Nov 21, 2014

@acastrounis, I didn't this post as saying hapi is improvement over Express, rather, based on what he said, it is more of a design choice of Hapi. Doing so avoid the problem that Express is having, but on the other hand, Hapi has to limit the type of routes, which may not work for some people.

In a more general case, I see that Express have a good default case, but if you are working with big teams with a lot of routes, then you might consider Hapi but you would have to give up some of the features that Express gave. At least that is the way that I see it.

karliky commented Nov 21, 2014

I think that the engineer who wrote the article was really tired of debugging an entire application that runs in production. I know the feeling of spend time investigating something unclear or hard to find, so I read the post with empathy of what he tries to transmit to the Node.js community.

A part from that, I agree with @hueniverse and the fact the Netflix team should have spent more time trying to understand why some engineering parts of Express.js are made "as is". There is no “best”.

I thought that Netflix was suitably humble in their admitting the not understanding of the library, but this post is great anyways.

dbgarf commented Nov 21, 2014

you guys are a bit oversensitive here. the post from netflix was oriented around their own need to learn more about their choice of technology so they use the API correctly. how you would interpret that as an attack on Express is beyond me.

It is incorrect that there aren't more asymptotically optimal solutions to the route matching problem (though netflix's post is also incorrect, in that there are certainly no constant time solutions to the route matching problem). You can convert all of the route patterns to regex's and then merge all of the regex's into a single DFA. That being said, this solution obviously comes at a significant code complexity cost, and is probably not worthwhile until you're in the hundreds or maybe even thousands of route handlers range.

LOL I see totally cultural difference :)

I didn't read it as an attack on Express at all. I think it was a nice writeup of how the engineer spotted a problem, analyzed it and came to a conclusion. It gave me insight in how Express handles routes that I most likely would never have looked into myself. Still, I appreciate this post as well and the interesting discussion that it inspired.

My concern is that this post by Netflix can cause fear, even if it benefits software engineers. One could very easily and prematurely come to the conclusion that "Node.js [is] in Flames" which might cause unnecessary panic. I believe whoever posted this at Netflix had good intentions and didn't mean to cause this much buzz; but some parts of the article, especially the title, are misleading and negative towards node and express.

rlidwka commented Nov 22, 2014

but some parts of the article, especially the title

The article is about flamegraphs, so the title is exactly right.

@baudehlo

I disagree that it would be worthwhile changing the way Express handles routing. As @hueniverse already pointed out, Restify handles it the same way. A typical express app should not have enough routes / middlewares to make the complexity trade-off worthwhile. If you find that you need a few thousand routes, maybe it's time to split the app into multiple apps and use a reverse proxy to help route to the appropriate place.

This Netflix article is the first time I've heard of anybody whose major app bottleneck was the router. Typically, the major bottlenecks in an Express app are I/O related.

I have extensive experience with both Restify and Express. If Netflix thinks they're out of the woods when it comes to framework quirks, they're about to learn different. Every framework has its issues.

OMG, next thing you know the engineer will be crying on TV and making an apology for offending so many people. I think Netflix wrote a fine article and I learned something today. And you should be ashamed because next time they'll think twice before sharing.

@ericelliott: As I said in my blog post, I don't have any issues with the performance personally. But express does serve many web sites, and putting performance tweaks into frameworks is the right place to do it so that everyone benefits.

dgomesbr commented Dec 3, 2014

Agreed with @svenanders. The post have shown how flame graph helps debugging your and others code. That they didn't understood how express worked internally and how they came to a solution. The comments about the algorithm per se are points of view of any software engineer should worry about like Big-O and Data Structures.

The problem with an article like the one Netflix posted is that it unnecessarily creates FUD around the adoption of NodeJs (and express) simply because of the way it was titled and worded. It's not really about the facts that were presented or whether they ultimately admitted their mistake. It's about the political and perceptual reality that exists out there in many organizations and in the development community as a whole. It would be nice if they would maybe add some kind of disclaimer or tl;dr at the beginning of the "inflammatory" post.

It should be "handicapped its" vs "handicapped it’s".

Takeaways:

  • when using express, do not muck with your live routing structure unless you really know what you're doing;
  • when using express, flatten your routing when you can, think about how the router stores the definitions and searches them for matches; Express has opted not to attempt a reconciliation of patterns in matcher expressions, so there will be looping;
  • don't write a blog post with an "inflammatory" title aimed in the wrong direction, or if you do, be prepared for pushback;
  • consider contributing improvements to open-source software (e.g. optimize the loop).

When I look at these bullet points, they all sound self-evident in retrospect. That said, I appreciated this analysis and that of the original post. It's not like there are ad hominem attacks going on (in this case)... :)

csvan commented May 28, 2015

Have any of the suggested performance improvements to the Express Router been implemented and merged since this response was written?

There are many ways to optimize route lookup, and the simple array iteration in Express simply means someone didn't do his algorithms and complexity analysis homework - at least if Express is meant to be used for large, rich backends, where hundreds of routes are an absolute normality, IMO, the more so if one single route adds 1 ms of overhead for searching - that's a huge amount of time. We do use high level languages for increased programmer productivity, but that doesn't mean we no longer need to know about data structures and algorithms, especially when we're writing framework/library code.

On the other hand, I think you've also miss-read Netflix' post. IMO, what they're saying is that express is not a fit as good as Restify for their needs. Express is a rich framework for building the server side of a web app, one with an html-based GUI, CSS, images, client-side JavaScript and so on. They don't need this, to my understanding. What they need is something simple, straightforward, performance-optimized, maybe even somewhat frugal, feature-wise, on which to build the simpler or the more often changing services of their infrastructure. That's simply not the target of Express, at least to my understanding. Even if routing would have been super-optimized in Express, Express would still not be a fit as good for their needs as Restify, or another framework specifically designed for developing APIs, not applications. Features that you don't use, in a framework, still add cost, therefore it makes sense, cost-wise, to choose the framework/library which provides what you need and only very little else.

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