-
-
Notifications
You must be signed in to change notification settings - Fork 660
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
perf(trie-router): optimize 2x faster #3732
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3732 +/- ##
==========================================
- Coverage 91.72% 91.71% -0.01%
==========================================
Files 159 159
Lines 10184 10181 -3
Branches 2990 2992 +2
==========================================
- Hits 9341 9338 -3
Misses 842 842
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Hi @EdamAme-x That's great! I have also confirmed that the following optimisations are also effective. Well, but I also think it's a little tricky. (() => {
const E = function () {};
E.prototype = Object.create(null);
return E;
})() Another simple way.Although we haven't tracked down the specific cause, I think we can say that your excellent investigation has revealed that hono/src/router/trie-router/node.ts Line 110 in 50ff212
Even with changes like the ones below, the performance will be the same as this PR, so I think this simple way is best if possible. diff --git a/src/router/trie-router/node.ts b/src/router/trie-router/node.ts
index 94ab4de1..da415a3a 100644
--- a/src/router/trie-router/node.ts
+++ b/src/router/trie-router/node.ts
@@ -82,7 +82,7 @@ export class Node<T> {
node: Node<T>,
method: string,
nodeParams: Record<string, string>,
- params: Record<string, string>
+ params?: Record<string, string>
): HandlerParamsSet<T>[] {
const handlerSets: HandlerParamsSet<T>[] = []
for (let i = 0, len = node.#methods.length; i < len; i++) {
@@ -95,7 +95,7 @@ export class Node<T> {
const key = handlerSet.possibleKeys[i]
const processed = processedSet[handlerSet.score]
handlerSet.params[key] =
- params[key] && !processed ? params[key] : nodeParams[key] ?? params[key]
+ params?.[key] && !processed ? params[key] : nodeParams[key] ?? params?.[key]
processedSet[handlerSet.score] = true
}
@@ -107,7 +107,7 @@ export class Node<T> {
search(method: string, path: string): [[T, Params][]] {
const handlerSets: HandlerParamsSet<T>[] = []
- this.#params = Object.create(null)
+ this.#params = {}
// eslint-disable-next-line @typescript-eslint/no-this-alias
const curNode: Node<T> = this
@@ -129,17 +129,10 @@ export class Node<T> {
// '/hello/*' => match '/hello'
if (nextNode.#children['*']) {
handlerSets.push(
- ...this.#getHandlerSets(
- nextNode.#children['*'],
- method,
- node.#params,
- Object.create(null)
- )
+ ...this.#getHandlerSets(nextNode.#children['*'], method, node.#params)
)
}
- handlerSets.push(
- ...this.#getHandlerSets(nextNode, method, node.#params, Object.create(null))
- )
+ handlerSets.push(...this.#getHandlerSets(nextNode, method, node.#params))
} else {
tempNodes.push(nextNode)
}
@@ -155,9 +148,7 @@ export class Node<T> {
if (pattern === '*') {
const astNode = node.#children['*']
if (astNode) {
- handlerSets.push(
- ...this.#getHandlerSets(astNode, method, node.#params, Object.create(null))
- )
+ handlerSets.push(...this.#getHandlerSets(astNode, method, node.#params))
tempNodes.push(astNode)
}
continue |
Oops, sorry, |
I had considered same approach in the past, but was concerned about unintended side effects caused by properties defined in prototypes such as constructor. This seems pretty magical, but it works well. |
@EdamAme-x Thanks for the reply! I researched afterward and found that the spread operator for import { run, bench, group } from 'mitata'
bench('noop', () => {})
const emptyParams: Readonly<Record<string, never>> = Object.create(null)
const OptimizedEmptyParams = (() => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
const E = function () {}
E.prototype = emptyParams
return E
// eslint-disable-next-line @typescript-eslint/no-explicit-any
})() as unknown as { new (): Record<string, any> }
group('spread operator', () => {
bench('Object.create(null)', () => {
const obj = Object.create(null)
return { ...obj }
})
bench('new OptimizedEmptyParams()', () => {
const obj = new OptimizedEmptyParams()
return { ...obj }
})
bench('{}', () => {
const obj = {}
return { ...obj }
})
})
run()
However, in the case of TrieRouter, I think it is better to optimize using standard coding methods rather than techniques based on special code. The following branch avoids the spread operator for main...usualoma:hono:perf-trie-1208
|
Thank you for your careful research. |
@EdamAme-x Thank you! I'd like to hear your opinion (I have no intention of confronting you). The perf-trie-1208 branch seems to be the best performing at the moment. For example, if we use a magical technique, will it perform better than perf-trie-1208? I made the following changes to perf-trie-1208 and measured the performance, but it did not improve. diff --git a/src/router/trie-router/node.ts b/src/router/trie-router/node.ts
index a580e0cb..f896ee47 100644
--- a/src/router/trie-router/node.ts
+++ b/src/router/trie-router/node.ts
@@ -14,6 +14,13 @@ type HandlerParamsSet<T> = HandlerSet<T> & {
}
const emptyParams = Object.create(null)
+const OptimizedEmptyParams = (() => {
+ // eslint-disable-next-line @typescript-eslint/no-empty-function
+ const E = function () {}
+ E.prototype = emptyParams
+ return E
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+})() as unknown as { new (): any }
export class Node<T> {
#methods: Record<string, HandlerSet<T>>[]
@@ -92,7 +99,7 @@ export class Node<T> {
const handlerSet = (m[method] || m[METHOD_NAME_ALL]) as HandlerParamsSet<T>
const processedSet: Record<number, boolean> = {}
if (handlerSet !== undefined) {
- handlerSet.params = Object.create(null)
+ handlerSet.params = new OptimizedEmptyParams()
handlerSets.push(handlerSet)
if (nodeParams) {
for (let i = 0, len = handlerSet.possibleKeys.length; i < len; i++) { |
To me, the |
…ull)` This optimization is based on the discussion in the following Pull Request. Thanks to @EdamAme-x! honojs#3732 Co-authored-by: EdamAmex <[email protected]>
Hi @EdamAme-x Thank you! Then, I would like to proceed with the following pull request by adding you as a co-author. What do you think, @yusukebe? |
I'll check it tomorrow! |
…ull)` (#3735) * perf(trie-router): avoid calling spread operator for `Object.create(null)` This optimization is based on the discussion in the following Pull Request. Thanks to @EdamAme-x! #3732 Co-authored-by: EdamAmex <[email protected]> * perf(trie-router): make some #getHandlerSets parameters optional * fix(trie-router): fix #getHandlerSets bug --------- Co-authored-by: EdamAmex <[email protected]>
TrieRouter
is now much faster!This PR does not include any attempt to reduce the file size, so a patch must be applied after the merge.
Speedups were achieved by using strange optimization methods to create null objects in JavaScript.
Benchmarks
In Node (1.1x faster)
In Deno (2x faster)
In Bun (1.5x faster)
The author should do the following, if applicable
bun run format:fix && bun run lint:fix
to format the code