refactor router

[jonathanong]

we were planning to do this in v4 but we never got around to it. @mscdex has been working on optimizing it and says he's gotten an 80% improvement so far. i guess this is the repo: https://github.com/mscdex/express-optimized

he says it may not be totally backwards compatible, so we'll see what we can do

[mscdex]

It's less than 80% now that I've added more compatible behavior, but here's what I currently get with the silly benchmark included with Express:

express-optimized

1 middleware
11271.28

5 middleware
10967.51

10 middleware
10666.83

15 middleware
10751.10

20 middleware
10529.86

30 middleware
10004.90

50 middleware
9654.49

100 middleware
8126.97

Express 4.5.0

1 middleware
5673.28

5 middleware
5354.22

10 middleware
5163.09

15 middleware
4907.19

20 middleware
4587.97

30 middleware
4104.19

50 middleware
3631.30

100 middleware
2569.67

[jonathanong]

how much of it is just using fast-url-parser?

[jonathanong]

actually nevermind, id on't see any path matching in the benchmarks.

[mscdex]

I'm not calling a url parser at the moment, just doing a simple substring() to separate the path and query string. This obviously affects req.urls like http://foo/bar/baz. Otherwise it uses the same 'path-to-regexp' for creating the path regexps.

[dougwilson]

Neat. I'd also be interested in like if you kept a fork of express with your router added into it so I could run the current test suite against it (really, just to know what it does and doesn't do right now).

[mscdex]

The readme describes exactly what is supported. I don't care much to muck around with Express's testing setup :-).

[dougwilson]

haha. I also have a question with the benchmark: when it was run against express 4.5.0, I assume it was run against new express.Router() and not against express(), is that right?

[mscdex]

The benchmark was ran as-is from the repo.

[dougwilson]

It also looks like the handlers are not wrapped in a try ... catch, which could be one of the biggest performance gains. Do you have any data regarding that as well? I'm not sure if we should remove the try ... catch stuff.

[mscdex]

If you have to do a try-catch, you should do it in a separate function that just does a try-catch since v8 will deoptimize the whole function that a try-catch is in.

[dougwilson]

Right. Can you add that to your router, perhaps? See how that hits the numbers is all I want (not saying there isn't a way to optimize it, just that it probably makes a difference in those benchmarks is all).

[mscdex]

Adding the try-catch doesn't affect things significantly:

  1 middleware
  11125.48

  5 middleware
  10850.55

  10 middleware
  10733.65

  15 middleware
  10526.39

  20 middleware
  10397.90

  30 middleware
  10005.28

  50 middleware
  9323.55

  100 middleware
  7972.72

[dougwilson]

So far I'm trying to use your router and doing standard .get stuff, but keep getting Error: Cannot mount a Router in multiple places for some reason... I'm trying to figure it out.

[defunctzombie]

We can rip the router stuff out into the routification repo and do perf/bench improvements there. I am all for perf gains as long as feature set is not affected.

[dougwilson]

@defunctzombie yes, I agree we should rip the router into a separate repo for sure--I had sort of started doing that a week ago, because I think @jonathanong was talking about we should do that for express 5.x.

[defunctzombie]

I took a look at the optimized code. IMO it is hard to follow and not well organized. Maybe the lessons learned can be applied to some "slow" router pieces or isolated. I would not favor moving over to that code as is. I realize it may be faster and that is something we can learn from. Optimized code is often less pretty than not, so it might be worth it for some parts but not as a whole.

[defunctzombie]

We should have the new repo be ready for use before express 5 lands so people can test that out for their router needs.

[dougwilson]

@defunctzombie so far it doesn't seem to even work for the most basic apps in our examples/ folder, so that optimized repo has quite a long way to go still, it would seem.

so people can test that out for their router needs.

?? I'm not 100% sure what that means, sorry :) I did want to have a module out that was exactly the express 4.x router, so if express 5.x has different mechanics, people could require a old version of that lib to use the same 4.x-compatible routing.

[defunctzombie]

I did want to have a module out that was exactly the express 4.x router, so if express 5.x has different mechanics, people could require a old version of that lib to use the same 4.x-compatible routing.

It means exactly that. I meant to say they could depend on the router module separately from express core.

[dougwilson]

haha, I guess we think similar :)

[hacksparrow]

Is there a roadmap planned for Express 5?

On Mon, Jul 7, 2014 at 10:44 AM, Douglas Christopher Wilson <
notifications@github.com> wrote:

haha, I guess we think similar :)

—
Reply to this email directly or view it on GitHub
#2218 (comment)
.

[dougwilson]

@hacksparrow not yet. We are working on one just recently :) I don't know if we're going to keep one in the wiki, or just have issues in the issue tracker only yet. I'm also planning a 5.x branch people can start watching and commenting on, etc.

[hacksparrow]

Either the wiki or the issue tracker should suffice, as long as we have a
picture of what's coming, in my opinion. Looking forward to the details.

On Mon, Jul 7, 2014 at 10:47 AM, Douglas Christopher Wilson <
notifications@github.com> wrote:

@hacksparrow https://github.com/hacksparrow not yet. We are working on
one just recently :) I don't know if we're going to keep one in the wiki,
or just have issues in the issue tracker only yet. I'm also planning a 5.x
branch people can start watching and commenting on, etc.

—
Reply to this email directly or view it on GitHub
#2218 (comment)
.

[dougwilson]

@hacksparrow yes. I don't think we want to make changes in the dark here :) A 5.x means something broke and people need to change their stuff. Breaking something should have a good reason and be open for discussion/oversight for some time, haha.

[mscdex]

So far I'm trying to use your router and doing standard .get stuff, but keep getting Error: Cannot mount a Router in multiple places for some reason... I'm trying to figure it out.

.

I took a look at the optimized code. IMO it is hard to follow and not well organized. Maybe the lessons learned can be applied to some "slow" router pieces or isolated. I would not favor moving over to that code as is. I realize it may be faster and that is something we can learn from. Optimized code is often less pretty than not, so it might be worth it for some parts but not as a whole.

I just started hacking on it this weekend for fun, so I haven't started writing tests yet. It's merely designed to be an experiment whose purpose is to show that Express currently isn't as "insanely fast" as it could/should be.

Separating out the router (if possible) is a good idea though.

[dougwilson]

I just started hacking on it this weekend for fun, so I haven't started writing tests yet. It's merely designed to be an experiment whose purpose is to show that Express currently isn't as "insanely fast" as it could/should be.

Yep, I know it's new and has a lot of missing pieces :) I'm just afraid on what the speed will end up being when the pieces are added in--it's hard to tell how much it'll slow down the router and in the end it could end up being comparable in speed instead of the large difference it is now, idk.

[dougwilson]

@mscdex Can you run benchmark in branch https://github.com/visionmedia/express/tree/slim-benchmark on your machine and output the results for express 4.5.1? I noticed that your benchmark was using res.end instead of res.send--res.send does a bunch of work, including calculation an ETag and other things.

[mscdex]

@dougwilson It gives a small bump in performance.

[dougwilson]

It gives a small bump in performance.

Cool. By any chance when you get around to it, can you run the benchmark in the latest https://github.com/visionmedia/express/tree/slim-benchmark branch? Sorry I keep asking ya, but I don't have wrk setup on my Windows machine, and the numbers are not comparable between different machines.

[dougwilson]

@hacksparrow and those interested, there is now a 5.x branch to watch: https://github.com/visionmedia/express/tree/5.x

[dougwilson]

This link makes more sense: https://github.com/visionmedia/express/compare/5.x

[mscdex]

@dougwilson I just got a chance to benchmark the slim-benchmark branch (Updated to include Express 4.6.1 merge):

  1 middleware
  11275.06

  5 middleware
  11112.06

  10 middleware
  10543.85

  15 middleware
  9992.34

  20 middleware
  9612.38

  30 middleware
  9018.85

  50 middleware
  7964.98

  100 middleware
  6141.41

Here is with express() instead of express.Router():

  1 middleware
  7656.20

  5 middleware
  7408.59

  10 middleware
  7250.11

  15 middleware
  6904.95

  20 middleware
  6754.87

  30 middleware
  6301.25

  50 middleware
  5845.25

  100 middleware
  4818.05

One of the things I'd like to see is for an Express "application" to really just be a Router instance. That would go some way in ensuring consistent performance.

[dougwilson]

Ooops, the slim-benchmark is over an entire release version behind! I just rebased the branch use use the current 4.6.1 code base, if you want to run it again.

[dougwilson]

One of the things I'd like to see is for an Express "application" to really just be a Router instance. That would go some way in ensuring consistent performance.

The main difference is that if you just use a router, it doesn't add a bunch of implicit middleware, alter the req and res prototypes and other things that the express instances are doing.

[mscdex]

Ok, I've updated the previous post with the new results. It looks like performance is starting to get better, however the performance seems to drop quite a bit (compared to express-optimized) after adding more and more middleware.

[dougwilson]

Charting the data from #2218 (comment) vs #2218 (comment) it looks like they drop at about the same rate to me:

Edit: used the wrong chart type the first time by accident :)

[mscdex]

The main difference is that if you just use a router, it doesn't add a bunch of implicit middleware, alter the req and res prototypes and other things that the express instances are doing.

What implicit middleware?
IMHO having express() not be a Router instance makes things really confusing, at least for me. This includes gotchas like .use() not working the same way in a Router vs express().

@dougwilson What I meant is that the performance diverges pretty quickly after 10 or so middleware.

[dougwilson]

What implicit middleware?

The two things in the lib/middleware folder are automatically added as the first two in the stack.

This includes gotchas like .use() not working the same way in a Router vs express().

How do they differ?

[dougwilson]

Two .use() is just a pass-through: https://github.com/visionmedia/express/blob/master/lib/application.js#L190

The main difference is the application mounting, which is a historical thing, but still very widely used by other people, mainly because they get the mount event and can then hook into the parent app.

[mscdex]

@dougwilson See this issue for the kind of thing I'm referring to.

[dougwilson]

See this issue for the kind of thing I'm referring to.

Um, that issue is for .route.use(), which doesn't even exist. routes are not the same as routers, nor the same as apps.

[mscdex]

That's even more confusing :-)

[dougwilson]

The main difference between a router and a route is that your handles in a router are executed linearly, so if you change the req.url or req.method inside a handler in a router, it gets essentially re-routed, while a route is a fixed tree off a req.url; changing the req.url or req.method inside that will not take effect until the route has been exited. It also give you a way to group your verbs together, so a HEAD to a URL can see no corresponding HEAD exists and runs it through the route as a GET and discards the body, giving automatic HEAD support for people without them writing it. It also allows you to group your methods such that OPTIONS knows how to respond without executing handlers (which can have side-effects and do things like change req.url and req.method), etc.

[mscdex]

RE: OPTIONS, can't we just make that opt-in or something? The default HEAD handler makes sense though.

[dougwilson]

RE: OPTIONS, can't we just make that opt-in or something? The default HEAD handler makes sense though.

I intend to make both these things at least configurable, because there are reasons people would want them on and reasons people would want them off. Right now you technically can out-out by making dump empty routes, but that's... dumb :D

[dougwilson]

Also, I'm glad to see the couple changes I made in 4.6 made such an enormous change in those benchmarks :)

[defunctzombie]

💌

[mscdex]

Now to get app.* routing methods that performant (or close) ;-)

[dougwilson]

Now to get app.* routing methods that performant (or close) ;-)

Yep, though I'm not sure how possible will be, especially since the main difference is the __proto__ change, which from what I hear is expensive. The only other difference has already been fixed on 5.x branch.

[Fishrock123]

@dougwilson what's the status of this now?

[dougwilson]

This is an issue that was for using @mscdex 's router, which made the biggest, change that would break a lot of middleware in the community: it choose not to rewrite req.url in .use() calls (mscdex/express-optimized#5 (comment)).