lukeed / trouter

:fish: A fast, small-but-mighty, familiar fish...errr, router*
MIT License
634 stars 23 forks source link

Add support for RegExp patterns with named capture groups #11

Closed jirutka closed 5 years ago

jirutka commented 5 years ago

The support of RegExp patterns with named capture groups will significantly increase flexiblity of Trouter – it will allow to express any complex patterns for the cases where the regexparam patterns are not enough.

I’ve run bench on Node v10.14.2 and this change doesn’t have any provable negative effect on the performance.

It will increase size of index.mjs by ~11 %.

 Filename         Filesize  (gzip)
-index.js          1.18 kB  519 B 
+index.js          1.33 kB  579 B
-index.mjs         1.17 kB  518 B
+index.mjs         1.32 kB  577 B
lukeed commented 5 years ago

Cool! I'm traveling today but will do a careful comb through it later -- performance testing too (I have some local suites) since Trouter is the foundation of Polka

Thanks

lukeed commented 5 years ago

Okay, this looks good. I haven't done any performance testing, but I will resume at a later date.

I think I'll make changes to regexparam so that the changes here are as bit simpler, and it will allow navaid to also allow custom, named RegExps for client-side routing. Please keep this PR open :)

Feeling a bit under the weather right now (probably due to traveling) so I'll have to resume another day... likely this weekend.

Thanks again~!

lukeed commented 5 years ago

Final size is 558B and unfortunately there is measurable performance impact, but it's been mitigated to the best of my (current) ability

paulocoghi commented 5 years ago

Final size is 558B and unfortunately there is measurable performance impact, but it's been mitigated to the best of my (current) ability

This is exactly my concern: performance reduction when we don't use RegExp.

lukeed commented 5 years ago

So, it actually changes very little. What I was seeing was actually due to the addition of the RegExp routes themselves inside the benchmark. Running them at all deoptimizes (this benchmark) so everything runs slower.

With the RegExp routes in place, the de-opt (again, specific to this benchmark) was showing GET / in the 8M to low-9M range – that's too much for variance alone to be responsible, hence the initial concern.

Here are the actual numbers of this PR (after removing the RegExp routes) compared to the master results:

// PR:
GET /                             x 10,041,866 ops/sec ±1.21% (91 runs sampled)
POST /users                       x 11,834,775 ops/sec ±2.29% (95 runs sampled)
GET /users/:id                    x  6,091,946 ops/sec ±1.97% (88 runs sampled)
PUT /users/:id/books/:title?      x  6,244,150 ops/sec ±0.18% (95 runs sampled)
DELETE /users/:id/books/:title    x  5,823,797 ops/sec ±2.08% (97 runs sampled)
HEAD /hello (all)                 x  9,993,971 ops/sec ±0.24% (94 runs sampled)

// Master:
GET /                             x 10,261,090 ops/sec ±1.33% (92 runs sampled)
POST /users                       x 12,664,413 ops/sec ±2.59% (93 runs sampled)
GET /users/:id                    x  6,010,399 ops/sec ±2.20% (87 runs sampled)
PUT /users/:id/books/:title?      x  6,138,320 ops/sec ±0.21% (97 runs sampled)
DELETE /users/:id/books/:title    x  5,556,368 ops/sec ±2.02% (92 runs sampled)
HEAD /hello (all)                 x  9,697,531 ops/sec ±0.36% (93 runs sampled)

The size of these numbers means that things seemingly vary greatly, while that may not always be the case. Another example: the final 3 routes of the PR results are "significantly" faster despite the fact that it perhaps shouldn't be. Just the way the cookie crumbles sometimes...

And, even if Trouter really did get 200-800k slower on those two routes, the truth is that no Node.js application will ever actually be missing those "200k to 800k" ops/sec anyway since it's impossible for the HTTP engine to even operate that quickly.

paulocoghi commented 5 years ago

Nice :)