5 ms·

Response to Netflix's “Node.js in Flames” Blog Post

130 points by joshuacc 12y ago

cleverjake 12y ago
Worth noting that the person defending express is Eran Hammer - the of one of it's biggest competitors, hapi (hapijs.com)
hardwaresofton 12y ago
I want to point out that in the thread here on HN (https://news.ycombinator.com/item?id=8631022 https://news.ycombinator.com/item?id=8631022) about that Netflix article, the top comment debunks the myth that the list-of-handlers was the issue slowing them down. While creating a grammar from the combination of the regular expressions (which is why you can't just have a hash) is definitely a solution (quite possibly THE solution), that's definitely not an MVP-type of solution, and is pretty non-trivial. The actual problem was them constantly adding handlers by accident. One of the reasons I love and use HN so much (I give almost no other site so much pull on my personal opinion) is that smart (or very careful and well-read dumb people) leave well-thought-out comments like the one I mentioned.
- kevingadd 12y ago
  This is all true. However, I can't take a stack seriously as an enterprise or production ready piece of software when it does something like iterate a list recursively in performance sensitive code. It's just bizarre. JS doesn't have tail call optimization and for loops are just fine. Being able to blow out the JS stack by having 'too many handlers' is terrifying to me, especially because JS runtimes have catastrophically poor support for handling and debugging stack overflows. The use of a single array for handlers that can contain multiple items also feels like an architectural red flag to me, but I can understand how that would happen since JS's standard library is so anemic and lacks useful data structures. What you really want is a set (or ordered set, if handler order is super important - it probably is here) I saw a number of entries in those flame charts that appeared to be V8 compilation internals, which is suspicious to me. But those are probably just debugger/symbol resolution issues, I hope, and not actual JIT recompilation constantly happening during route dispatch? (If v8 were compiling code constantly during dispatch, that would certainly explain multiple-ms pauses...)
  - thedufer 12y ago
    > However, I can't take a stack seriously as an enterprise or production ready piece of software when it does something like iterate a list recursively in performance sensitive code. It's just bizarre. JS doesn't have tail call optimization and for loops are just fine. Can you explain how you would do asynchronous tasks in series without recursion? For loops successfully handle this when either the tasks are synchronous or can be run in parallel. Neither of those are true here.
    - kevingadd 12y ago
      Virtually all real-world stacks I know of use a work queue where each element in the list is queued up once it's ready. You don't need on-stack recursion or unbounded memory to track the execution states. It's pretty trivial to implement this given a promises API and any sort of event queue, but you could also just do it with callbacks. So in the browser you'd literally just use setTimeout. One reason this is so common is that it's trivial to end up blowing out the stack with recursive IO callbacks, because sometimes all your read()s complete instantly because all the data's in cache. Oops!
  - hardwaresofton 12y ago
    How would you handle their problem? Maybe this can turn into a chance to write some code (I'm willing to) to improve express?
    - kevingadd 12y ago
      See this post for an explanation: https://news.ycombinator.com/item?id=8632604 https://news.ycombinator.com/item?id=8632604 The 'push ready callbacks onto a queue' model is really common and relatively straightforward to optimize. I've used it in a variety of applications and it can provide very good throughput in both single and multiple threaded scenarios.
      - hardwaresofton 12y ago
        I did read that comment of yours, to make it more concrete -- 1. Create some sort of handler execution queue 2. Push first handler onto queue 3. When a handler runs, which may translate into the firing of another handler/ translation of the route, push a "handle callback" (for lack of a better term) onto the queue 4. Return to #3 and continue forever (which is a possibility) or run out of handlers at some point (be done)
ericHosick 12y ago
> 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. Wouldn't a better solution, in this context, be to aggregate values under the same key?
jongleberry 12y ago
this is why the express team actively rejects any features or support for dynamically changing middleware and routes during runtime.
- _RPM 12y ago
  _this_ is ambiguous. Can you explain what the context of "this" is?
  - fraserharris 12y ago
    From the original Netflix post: "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. Unfortunately, it was also inadvertently adding a static route handler with the same path each time it ran."
  - VeejayRampay 12y ago
    The ultimate irony is probably a comment about the ambiguity of "this" in an article about a Javascript-based technology.
coldcode 12y ago
It does go to show you that you need to understand the limitations of a hammer before starting to whack things with it. This is true of any framework or library you use. Even more necessary if you are processing billions of things with it.
- freshflowers 12y ago
  It's not so black and white. In many cases it's faster and cheaper (and more educational) to just start whacking and find out what the limitations are. Of course when you're operating on the scale of Netflix then, yeah, sure.
cjensen 12y ago
Is there a reason why they don't have a map for static strings, and use an array for regexs?
- trevordixon 12y ago
  Probably makes it hard to do things in the right order. You'd have a separate array just to keep track of order that you'd be iterating every time. And express allows for more than one handler even for a static route. app.get('/', function(req, res, next) { next(); }); app.get('/', function(req, res) { res.send(); });
  - danudey 12y ago
    This could be solved by having (internally) a map of { path: [handler 1, handler 2]}. That makes static path matching constant time, and static paths with multiple handlers iterate over the requests appropriately. This can be a little confusing, especially given that you'd be specifying static and non-static paths separately, but it could be argued that that is less confusing than some methods. For example, nginx's method allows you to define paths in any order, but checks routes matching regular expressions first in config order, then picks a static route by most specific match; IOW the order of your routes in nginx doesn't matter until you have regular expressions, in which case it is surprisingly critical some of the time and irrelevant the rest of the time. Breaking it up into 'regular expression routes (checked first)' and 'static routes (checked last)' could simplify things conceptually, but doubtlessly raises more usability issues as well.
- Jare 12y ago
  To maintain ordering I assume.
  - TheLoneWolfling 12y ago
    There is such a beastie as an ordered hashmap.

130 points by joshuacc 12y ago

Worth noting that the person defending express is Eran Hammer - the of one of it's biggest competitors, hapi (hapijs.com)

hardwaresofton 12y ago

I want to point out that in the thread here on HN (https://news.ycombinator.com/item?id=8631022 https://news.ycombinator.com/item?id=8631022) about that Netflix article, the top comment debunks the myth that the list-of-handlers was the issue slowing them down. While creating a grammar from the combination of the regular expressions (which is why you can't just have a hash) is definitely a solution (quite possibly THE solution), that's definitely not an MVP-type of solution, and is pretty non-trivial. The actual problem was them constantly adding handlers by accident. One of the reasons I love and use HN so much (I give almost no other site so much pull on my personal opinion) is that smart (or very careful and well-read dumb people) leave well-thought-out comments like the one I mentioned.

kevingadd 12y ago
This is all true. However, I can't take a stack seriously as an enterprise or production ready piece of software when it does something like iterate a list recursively in performance sensitive code. It's just bizarre. JS doesn't have tail call optimization and for loops are just fine. Being able to blow out the JS stack by having 'too many handlers' is terrifying to me, especially because JS runtimes have catastrophically poor support for handling and debugging stack overflows. The use of a single array for handlers that can contain multiple items also feels like an architectural red flag to me, but I can understand how that would happen since JS's standard library is so anemic and lacks useful data structures. What you really want is a set (or ordered set, if handler order is super important - it probably is here) I saw a number of entries in those flame charts that appeared to be V8 compilation internals, which is suspicious to me. But those are probably just debugger/symbol resolution issues, I hope, and not actual JIT recompilation constantly happening during route dispatch? (If v8 were compiling code constantly during dispatch, that would certainly explain multiple-ms pauses...)
- thedufer 12y ago
  > However, I can't take a stack seriously as an enterprise or production ready piece of software when it does something like iterate a list recursively in performance sensitive code. It's just bizarre. JS doesn't have tail call optimization and for loops are just fine. Can you explain how you would do asynchronous tasks in series without recursion? For loops successfully handle this when either the tasks are synchronous or can be run in parallel. Neither of those are true here.
  - kevingadd 12y ago
    Virtually all real-world stacks I know of use a work queue where each element in the list is queued up once it's ready. You don't need on-stack recursion or unbounded memory to track the execution states. It's pretty trivial to implement this given a promises API and any sort of event queue, but you could also just do it with callbacks. So in the browser you'd literally just use setTimeout. One reason this is so common is that it's trivial to end up blowing out the stack with recursive IO callbacks, because sometimes all your read()s complete instantly because all the data's in cache. Oops!
- hardwaresofton 12y ago
  How would you handle their problem? Maybe this can turn into a chance to write some code (I'm willing to) to improve express?
  - kevingadd 12y ago
    See this post for an explanation: https://news.ycombinator.com/item?id=8632604 https://news.ycombinator.com/item?id=8632604 The 'push ready callbacks onto a queue' model is really common and relatively straightforward to optimize. I've used it in a variety of applications and it can provide very good throughput in both single and multiple threaded scenarios.
    - hardwaresofton 12y ago
      I did read that comment of yours, to make it more concrete -- 1. Create some sort of handler execution queue 2. Push first handler onto queue 3. When a handler runs, which may translate into the firing of another handler/ translation of the route, push a "handle callback" (for lack of a better term) onto the queue 4. Return to #3 and continue forever (which is a possibility) or run out of handlers at some point (be done)

ericHosick 12y ago

> 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. Wouldn't a better solution, in this context, be to aggregate values under the same key?

jongleberry 12y ago

this is why the express team actively rejects any features or support for dynamically changing middleware and routes during runtime.

_RPM 12y ago
_this_ is ambiguous. Can you explain what the context of "this" is?
- fraserharris 12y ago
  From the original Netflix post: "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. Unfortunately, it was also inadvertently adding a static route handler with the same path each time it ran."
- VeejayRampay 12y ago
  The ultimate irony is probably a comment about the ambiguity of "this" in an article about a Javascript-based technology.

coldcode 12y ago

It does go to show you that you need to understand the limitations of a hammer before starting to whack things with it. This is true of any framework or library you use. Even more necessary if you are processing billions of things with it.

freshflowers 12y ago
It's not so black and white. In many cases it's faster and cheaper (and more educational) to just start whacking and find out what the limitations are. Of course when you're operating on the scale of Netflix then, yeah, sure.

cjensen 12y ago

Is there a reason why they don't have a map for static strings, and use an array for regexs?

trevordixon 12y ago
Probably makes it hard to do things in the right order. You'd have a separate array just to keep track of order that you'd be iterating every time. And express allows for more than one handler even for a static route. app.get('/', function(req, res, next) { next(); }); app.get('/', function(req, res) { res.send(); });
- danudey 12y ago
  This could be solved by having (internally) a map of { path: [handler 1, handler 2]}. That makes static path matching constant time, and static paths with multiple handlers iterate over the requests appropriately. This can be a little confusing, especially given that you'd be specifying static and non-static paths separately, but it could be argued that that is less confusing than some methods. For example, nginx's method allows you to define paths in any order, but checks routes matching regular expressions first in config order, then picks a static route by most specific match; IOW the order of your routes in nginx doesn't matter until you have regular expressions, in which case it is surprisingly critical some of the time and irrelevant the rest of the time. Breaking it up into 'regular expression routes (checked first)' and 'static routes (checked last)' could simplify things conceptually, but doubtlessly raises more usability issues as well.
Jare 12y ago
To maintain ordering I assume.
- TheLoneWolfling 12y ago
  There is such a beastie as an ordered hashmap.