delvedor / find-my-way

A crazy fast HTTP router
MIT License
1.46k stars 133 forks source link

Potential performance improvements #371

Closed JaoodxD closed 3 months ago

JaoodxD commented 4 months ago

Just found hono router benchmarks which shows that find-my-way is 1.15x to 2.2x slower than hono's one in various scenarios.
Could these benchmarks and hono router itself become a good material/background for future improvements?

Link for benchmarks: https://github.com/honojs/hono/tree/main/benchmarks/routers Link for hono routers codebase: https://github.com/honojs/hono/tree/main/src/router

ivan-tymoshenko commented 4 months ago

This might be interesting. At the very least, IMHO, it's worth investigating. Thanks.

mcollina commented 4 months ago

Part of the reasons why we have stopped optimizing the router is that the bottlenecks of Fastify were elsewhere.

The current code completely inlines when running on Fastify.

On our "imprecise" benchmarks, this results in Fastify being one of the fastest solution you can use for Node.js: https://github.com/fastify/benchmarks.


Note that the Hono benchmarks have a significant flaw: they bench creation time and lookup time together. This is necessary for Hono main target (CF), but I would expect different results if creation was taken into consideration: we never optimized it.

I'll see if my hunch is true and there is a different story there ;).

JaoodxD commented 4 months ago

Note that the Hono benchmarks have a significant flaw: they bench creation time and lookup time together.

Maybe I misunderstood something but what I've seen is that all of the routers was initialized during import phase like find-my-way here and wrapped into the object with generic match method for further unified benchmarking.

for (const route of routes) {
  router.on(route.method as HTTPMethod, route.path, handler)
}

export const findMyWayRouter: RouterInterface = {
  name,
  match: (route) => {
    router.find(route.method as HTTPMethod, route.path)
  },
}

I didn't see any re-creation there, just router.find calls enclosed into match method.
Could you please confirm if my interpretation aligns with the concerns raised, or if there's a specific aspect of the benchmarks that requires further attention?

mcollina commented 4 months ago

Here the numbers I see:

➜  router-benchmark git:(master) ✗ node benchmarks/find-my-way.js

=======================
 find-my-way benchmark
=======================
short static: 29,915,377 ops/sec
static with same radix: 10,270,644 ops/sec
dynamic route: 5,660,527 ops/sec
mixed static dynamic: 6,906,525 ops/sec
long static: 7,068,578 ops/sec
wildcard: 8,616,799 ops/sec
all together: 1,268,174 ops/sec
➜  router-benchmark git:(master) ✗ node benchmarks/hono-regexp.js

=======================
 Hono RegExp benchmark
=======================
short static: 56,724,850 ops/sec
static with same radix: 78,753,589 ops/sec
dynamic route: 12,411,670 ops/sec
mixed static dynamic: 13,105,194 ops/sec
long static: 77,740,096 ops/sec
wildcard: 13,906,086 ops/sec
all together: 3,796,498 ops/sec
➜  router-benchmark git:(master) ✗ node benchmarks/hono-trie.js

===========================
 Hono TrieRouter benchmark
===========================
short static: 4,537,843 ops/sec
static with same radix: 4,234,414 ops/sec
dynamic route: 1,112,388 ops/sec
mixed static dynamic: 2,290,115 ops/sec
long static: 3,000,909 ops/sec
wildcard: 3,062,192 ops/sec
all together: 502,144 ops/sec

Hono has a fast router that does not support all patterns and a slow router that supports all patterns. find-my-way supports all patterns.

Now, if you add routes like these:

        router.add('GET', '/reg-exp/router', 'foo')
        router.add('GET', '/reg-exp/:id', 'bar')

Hono needs to revert to using the "slow" router, which is implemented in their "smart" router, tanking the performance.

There are more of those here.

(find-my-way is not affected by these problems).


I personally don't like adding those kinds of traps in the source code of my libraries, because developers can easily fall into them and have really bad performance as a result.

JaoodxD commented 4 months ago

Amazing investigation, @mcollina

gurgunday commented 2 months ago

What about radix3, or rather h3?

In this benchmark, h3 outperforms Fastify by quite a bit

I didn't have time to look into this, just asking if anyone has investigated it before

mcollina commented 2 months ago

@gurgunday https://github.com/fastify/benchmarks/pull/324

gurgunday commented 2 months ago

Awesome, thanks!