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.

Sign up for GitHub

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

Jump to bottom

fix: fastify issue 3129 #194

Merged
merged 1 commit into from
Jun 15, 2021
Merged

fix: fastify issue 3129 #194

merged 1 commit into from
Jun 15, 2021

Conversation

climba03003
Copy link
Contributor

@climba03003 climba03003 commented Jun 11, 2021 •
edited
Loading

Fixes: fastify/fastify#3129

Before

lookup static route x 60,585,458 ops/sec ±0.41% (590 runs sampled)
lookup dynamic route x 3,180,845 ops/sec ±0.34% (592 runs sampled)
lookup dynamic multi-parametric route x 1,888,387 ops/sec ±0.18% (592 runs sampled)
lookup dynamic multi-parametric route with regex x 1,372,640 ops/sec ±0.22% (592 runs sampled)
lookup long static route x 3,070,287 ops/sec ±0.24% (589 runs sampled)
lookup long dynamic route x 2,317,857 ops/sec ±0.30% (590 runs sampled)
lookup static route on constrained router x 17,111,003 ops/sec ±0.80% (570 runs sampled)
lookup static versioned route x 2,786,721 ops/sec ±0.42% (589 runs sampled)
lookup static constrained (version & host) route x 2,759,036 ops/sec ±0.42% (589 runs sampled)
find static route x 41,397,708 ops/sec ±0.51% (572 runs sampled)
find dynamic route x 3,685,946 ops/sec ±0.30% (590 runs sampled)
***find dynamic parametric route with wildcard x 1,856,440 ops/sec ±0.36% (593 runs sampled)***
find dynamic multi-parametric route x 2,057,872 ops/sec ±0.27% (592 runs sampled)
find dynamic multi-parametric route with regex x 1,439,799 ops/sec ±0.26% (591 runs sampled)
find long static route x 4,112,700 ops/sec ±0.27% (591 runs sampled)
find long dynamic route x 2,859,143 ops/sec ±0.21% (593 runs sampled)
find long nested dynamic route x 1,213,767 ops/sec ±0.24% (592 runs sampled)
find long nested dynamic route with other method x 2,362,406 ops/sec ±0.26% (593 runs sampled)
find long nested dynamic route x 1,212,952 ops/sec ±0.22% (593 runs sampled)
find long nested dynamic route with other method x 2,401,196 ops/sec ±0.22% (593 runs sampled)
Done in 668.15s.

After

lookup static route x 60,103,328 ops/sec ±0.41% (592 runs sampled)
lookup dynamic route x 3,322,131 ops/sec ±0.25% (591 runs sampled)
lookup dynamic multi-parametric route x 1,805,812 ops/sec ±0.60% (590 runs sampled)
lookup dynamic multi-parametric route with regex x 1,359,185 ops/sec ±0.38% (591 runs sampled)
lookup long static route x 3,040,311 ops/sec ±0.32% (588 runs sampled)
lookup long dynamic route x 2,278,720 ops/sec ±0.26% (591 runs sampled)
lookup static route on constrained router x 17,422,556 ops/sec ±0.54% (575 runs sampled)
lookup static versioned route x 2,804,400 ops/sec ±0.23% (592 runs sampled)
lookup static constrained (version & host) route x 2,809,921 ops/sec ±0.18% (593 runs sampled)
find static route x 41,319,116 ops/sec ±0.62% (567 runs sampled)
find dynamic route x 3,652,912 ops/sec ±0.18% (591 runs sampled)
***find dynamic parametric route with wildcard x 1,829,294 ops/sec ±0.28% (593 runs sampled)***
find dynamic multi-parametric route x 2,114,606 ops/sec ±0.27% (593 runs sampled)
find dynamic multi-parametric route with regex x 1,491,503 ops/sec ±0.27% (593 runs sampled)
find long static route x 3,992,056 ops/sec ±0.47% (591 runs sampled)
find long dynamic route x 2,859,343 ops/sec ±0.24% (591 runs sampled)
find long nested dynamic route x 1,242,054 ops/sec ±0.19% (593 runs sampled)
find long nested dynamic route with other method x 2,414,779 ops/sec ±0.21% (593 runs sampled)
find long nested dynamic route x 1,235,653 ops/sec ±0.15% (593 runs sampled)
find long nested dynamic route with other method x 2,411,097 ops/sec ±0.17% (593 runs sampled)
Done in 665.96s.

@climba03003 climba03003 mentioned this pull request Jun 11, 2021
Closed
2 tasks
climba03003
climba03003 commented Jun 11, 2021
View reviewed changes
index.js
if (node === null) return null
var decoded = fastDecode(path.slice(-len))
if (decoded === null) {
return this.onBadUrl !== null
? this._onBadUrl(path.slice(-len))
: null
}
var handle = node.handlers[0]

var handle = derivedConstraints !== undefined ? node.getMatchingHandler(derivedConstraints) : node.unconstrainedHandler
Copy link
Contributor Author

@climba03003 climba03003 Jun 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the same pattern at https://github.com/delvedor/find-my-way/blob/master/index.js#L380 for consistency. And only unconstrainedHandler contains what we need to compute paramsObj.

mcollina
mcollina approved these changes Jun 11, 2021
View reviewed changes
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

happypoulp
happypoulp approved these changes Jun 14, 2021
View reviewed changes
Copy link

@happypoulp happypoulp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪 LGTM, thanks !

delvedor
delvedor approved these changes Jun 15, 2021
View reviewed changes
Copy link
Owner

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina merged commit 915456f into delvedor:master Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delvedor delvedor delvedor approved these changes

@happypoulp happypoulp happypoulp approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing param when mixing parametric and wildcard routes
4 participants
@climba03003 @mcollina @happypoulp @delvedor