Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential performance improvements #371

Closed
JaoodxD opened this issue May 13, 2024 · 5 comments
Closed

Potential performance improvements #371

JaoodxD opened this issue May 13, 2024 · 5 comments

Comments

@JaoodxD
Copy link

JaoodxD commented May 13, 2024

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
Copy link
Collaborator

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

@mcollina
Copy link
Collaborator

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
Copy link
Author

JaoodxD commented May 14, 2024

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
Copy link
Collaborator

mcollina commented May 14, 2024

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
Copy link
Author

JaoodxD commented May 14, 2024

Amazing investigation, @mcollina

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants