-
Notifications
You must be signed in to change notification settings - Fork 8k
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
fix #2762 #2767
fix #2762 #2767
Conversation
Detailed content: #2762 |
@appleboy @rw-access Would you please take a look when you are free? Very thx. 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some more comments and/or an explanation of how this works? it's not clear what's going on and that makes it hard to review
Please also add benchmark result like #2706 (comment) comment. |
hi,local 'go test --run TestTreeWildcard/TestRunDynamicRouting' passed and add benchmark if there are omissions, please help me to add. 😆 The previous code could not match when running 'benchmark', and 404 not found occurred,Previously, the matching of 'level 1 router' was unreliable, so the matching method was changed😆 The benchmarks: this PR - https://travis-ci.com/github/qm012/go-http-routing-benchmark/builds/231071258 Thank you all for your reviews. |
@qm012 Please provide the benchmark between master and PR and show the result of |
@qm012 I make the PR https://github.com/qm012/go-http-routing-benchmark/pull/1/files to change the go version and update the testing count. Please update the new result by $ benchstat old.txt new.txt
name old time/op new time/op delta
Gin_Param 72.0ns ± 0% 73.4ns ± 0% ~ (p=1.000 n=1+1)
Gin_Param5 122ns ± 0% 146ns ± 0% ~ (p=1.000 n=1+1)
Gin_Param20 320ns ± 0% 432ns ± 0% ~ (p=1.000 n=1+1)
Gin_ParamWrite 127ns ± 0% 130ns ± 0% ~ (p=1.000 n=1+1)
Gin_GithubStatic 91.2ns ± 0% 130.0ns ± 0% ~ (p=1.000 n=1+1)
Gin_GithubParam 160ns ± 0% 238ns ± 0% ~ (p=1.000 n=1+1)
Gin_GithubAll 31.9µs ± 0% 48.1µs ± 0% ~ (p=1.000 n=1+1)
Gin_GPlusStatic 65.4ns ± 0% 95.3ns ± 0% ~ (p=1.000 n=1+1)
Gin_GPlusParam 93.7ns ± 0% 137.0ns ± 0% ~ (p=1.000 n=1+1)
Gin_GPlus2Params 119ns ± 0% 153ns ± 0% ~ (p=1.000 n=1+1)
Gin_GPlusAll 1.29µs ± 0% 1.73µs ± 0% ~ (p=1.000 n=1+1)
Gin_ParseStatic 66.0ns ± 0% 85.2ns ± 0% ~ (p=1.000 n=1+1)
Gin_ParseParam 75.8ns ± 0% 86.4ns ± 0% ~ (p=1.000 n=1+1)
Gin_Parse2Params 87.5ns ± 0% 103.0ns ± 0% ~ (p=1.000 n=1+1)
Gin_ParseAll 2.23µs ± 0% 2.92µs ± 0% ~ (p=1.000 n=1+1)
Gin_StaticAll 24.1µs ± 0% 36.2µs ± 0% ~ (p=1.000 n=1+1) |
hi, @appleboy Thanks you for reminding me. master https://travis-ci.com/github/qm012/go-http-routing-benchmark/builds/231194542 The $ benchstat old.txt new.txt
name old time/op new time/op delta
Gin_Param 91.1ns ± 2% 119.2ns ± 4% +30.92% (p=0.000 n=10+10)
Gin_Param5 163ns ± 2% 253ns ± 3% +54.70% (p=0.000 n=9+10)
Gin_Param20 399ns ± 2% 712ns ± 8% +78.57% (p=0.000 n=9+10)
Gin_ParamWrite 174ns ± 1% 219ns ± 5% +26.21% (p=0.000 n=10+9)
Gin_GithubStatic 129ns ± 1% 223ns ± 1% +72.08% (p=0.000 n=9+9)
Gin_GithubParam 229ns ± 2% 429ns ± 2% +87.68% (p=0.000 n=10+9)
Gin_GithubAll 42.8µs ± 2% 73.0µs ± 2% +70.56% (p=0.000 n=9+10)
Gin_GPlusStatic 91.5ns ± 2% 147.0ns ± 2% +60.60% (p=0.000 n=10+9)
Gin_GPlusParam 137ns ± 2% 209ns ± 1% +53.05% (p=0.000 n=10+8)
Gin_GPlus2Params 180ns ± 1% 286ns ± 2% +59.00% (p=0.000 n=9+9)
Gin_GPlusAll 1.82µs ± 2% 2.86µs ± 1% +57.30% (p=0.000 n=10+10)
Gin_ParseStatic 99.3ns ± 3% 146.9ns ± 4% +47.92% (p=0.000 n=10+10)
Gin_ParseParam 106ns ± 1% 143ns ± 1% +35.59% (p=0.000 n=9+8)
Gin_Parse2Params 127ns ± 2% 183ns ± 3% +43.87% (p=0.000 n=10+10)
Gin_ParseAll 3.15µs ± 2% 4.90µs ± 2% +55.56% (p=0.000 n=10+10)
Gin_StaticAll 31.6µs ± 1% 59.4µs ± 2% +87.94% (p=0.000 n=9+10) |
Benchstat report of the newly submitted Code: $ benchstat old.txt new.txt
name old time/op new time/op delta
Gin_Param 93.5ns ± 5% 99.5ns ± 0% +6.49% (p=0.000 n=10+7)
Gin_Param5 166ns ± 2% 216ns ± 1% +30.03% (p=0.000 n=10+9)
Gin_Param20 407ns ± 2% 612ns ± 1% +50.42% (p=0.000 n=10+9)
Gin_ParamWrite 177ns ± 2% 187ns ± 2% +5.51% (p=0.000 n=9+9)
Gin_GithubStatic 135ns ± 1% 193ns ± 2% +43.04% (p=0.000 n=9+10)
Gin_GithubParam 236ns ± 5% 367ns ± 1% +55.40% (p=0.000 n=10+9)
Gin_GithubAll 43.8µs ± 2% 62.1µs ± 3% +41.81% (p=0.000 n=10+9)
Gin_GPlusStatic 93.9ns ± 3% 123.6ns ± 2% +31.58% (p=0.000 n=9+9)
Gin_GPlusParam 139ns ± 1% 184ns ± 2% +31.82% (p=0.000 n=9+9)
Gin_GPlus2Params 183ns ± 1% 246ns ± 2% +34.41% (p=0.000 n=10+8)
Gin_GPlusAll 1.84µs ± 2% 2.49µs ± 2% +35.36% (p=0.000 n=10+9)
Gin_ParseStatic 93.2ns ± 1% 131.3ns ± 1% +40.90% (p=0.000 n=10+9)
Gin_ParseParam 108ns ± 2% 129ns ± 1% +20.13% (p=0.000 n=10+8)
Gin_Parse2Params 133ns ± 3% 160ns ± 1% +20.37% (p=0.000 n=9+9)
Gin_ParseAll 3.11µs ± 1% 4.32µs ± 2% +38.70% (p=0.000 n=8+10)
Gin_StaticAll 32.3µs ± 2% 51.2µs ± 1% +58.16% (p=0.000 n=10+10) I will continue to optimize the report, but the current result is not very ideal // match '/', If this condition is matched, the next route is found
if strings.HasSuffix(n.path, "/") || strings.Contains(n.fullPath, ":") || n.path == "" {
matchNum++
} Thank you all for your reviews.😆 |
The benchmarks: The name old time/op new time/op delta
Gin_Param 62.5ns ± 4% 64.0ns ± 5% ~ (p=0.110 n=10+10)
Gin_Param5 110ns ± 3% 114ns ± 5% +3.31% (p=0.017 n=10+10)
Gin_Param20 276ns ± 2% 298ns ± 3% +7.92% (p=0.000 n=10+10)
Gin_ParamWrite 118ns ± 2% 120ns ± 5% ~ (p=0.136 n=10+10)
Gin_GithubStatic 78.3ns ± 2% 96.4ns ± 2% +23.07% (p=0.000 n=10+10)
Gin_GithubParam 129ns ± 0% 177ns ± 2% +37.55% (p=0.000 n=10+10)
Gin_GithubAll 25.5µs ± 0% 32.1µs ± 1% +26.01% (p=0.000 n=10+10)
Gin_GPlusStatic 62.7ns ± 0% 69.5ns ± 2% +10.90% (p=0.000 n=10+10)
Gin_GPlusParam 88.6ns ± 3% 105.4ns ± 2% +18.98% (p=0.000 n=10+10)
Gin_GPlus2Params 112ns ± 2% 138ns ± 1% +23.81% (p=0.000 n=10+10)
Gin_GPlusAll 1.14µs ± 1% 1.38µs ± 1% +20.95% (p=0.000 n=10+9)
Gin_ParseStatic 59.2ns ± 0% 67.1ns ± 2% +13.25% (p=0.000 n=10+10)
Gin_ParseParam 70.6ns ± 4% 75.7ns ± 2% +7.22% (p=0.000 n=10+10)
Gin_Parse2Params 83.3ns ± 1% 90.8ns ± 1% +8.95% (p=0.000 n=9+10)
Gin_ParseAll 1.97µs ± 1% 2.27µs ± 1% +15.50% (p=0.000 n=10+10)
Gin_StaticAll 18.6µs ± 0% 25.0µs ± 1% +34.31% (p=0.000 n=10+10) |
add more comments update comment add example fix benchmark not found add comment and update test method gin_integration_test.go#L407 update comment and lastedNode directly assign current node optimize code Optimize the search next router logic optimize code Adjust the matching rules Adjust the matching order update condition code
hi @appleboy I updated the code, and when looking for the contents of the test report, I found that the relevant code submitted with (#2706) maintained two nodes, which would cause the test report to fail to get the expected results, so I made changes and optimizations. By maintaining a node, I controlled the way to find nodes, and made several cross-test reports for comparison when judging test conditions, and got an acceptable final result. Thank you for your help and review. The benchmarks:
The benchstat report: name old time/op new time/op delta
Gin_Param 63.5ns ± 4% 62.6ns ± 4% ~ (p=0.448 n=10+10)
Gin_Param5 112ns ± 6% 114ns ± 3% ~ (p=0.109 n=10+10)
Gin_Param20 282ns ± 3% 297ns ± 1% +5.47% (p=0.000 n=10+10)
Gin_ParamWrite 119ns ± 5% 118ns ± 2% ~ (p=0.566 n=10+10)
Gin_GithubStatic 81.2ns ± 6% 79.3ns ± 0% ~ (p=0.085 n=10+10)
Gin_GithubParam 128ns ± 6% 132ns ± 1% +2.64% (p=0.006 n=10+10)
Gin_GithubAll 26.2µs ± 3% 27.1µs ± 1% +3.10% (p=0.004 n=10+10)
Gin_GPlusStatic 66.0ns ± 3% 63.3ns ± 2% -4.17% (p=0.000 n=10+10)
Gin_GPlusParam 91.2ns ± 3% 86.1ns ± 1% -5.62% (p=0.000 n=10+10)
Gin_GPlus2Params 117ns ± 5% 110ns ± 1% -6.32% (p=0.000 n=9+10)
Gin_GPlusAll 1.23µs ± 7% 1.22µs ± 0% ~ (p=1.000 n=10+8)
Gin_ParseStatic 62.4ns ± 5% 61.9ns ± 1% ~ (p=0.218 n=10+10)
Gin_ParseParam 70.2ns ± 6% 68.3ns ± 1% ~ (p=0.184 n=10+10)
Gin_Parse2Params 82.5ns ± 4% 83.7ns ± 1% +1.49% (p=0.014 n=10+9)
Gin_ParseAll 2.01µs ± 3% 2.05µs ± 2% ~ (p=0.287 n=10+10)
Gin_StaticAll 18.9µs ± 3% 19.4µs ± 1% +2.93% (p=0.000 n=10+10) |
@g1eny0ung Please also take look at it. #2767 (comment) |
I'll take a look these few days. Sorry because I'm a little busy this week. 😵 |
hi @appleboy After a lot of cross-testing and comparison, the latest feedback test report is very exciting.I also tried to match '/' in another way, but the final result was not satisfactory. The latest benchmarks:
The latest benchstat report: name old time/op new time/op delta
Gin_Param 63.5ns ± 4% 60.6ns ± 1% -4.54% (p=0.000 n=10+10)
Gin_Param5 112ns ± 6% 103ns ± 1% -7.63% (p=0.000 n=10+9)
Gin_Param20 282ns ± 3% 259ns ± 3% -7.91% (p=0.000 n=10+10)
Gin_ParamWrite 119ns ± 5% 117ns ± 3% ~ (p=0.160 n=10+10)
Gin_GithubStatic 81.2ns ± 6% 71.9ns ± 2% -11.36% (p=0.000 n=10+10)
Gin_GithubParam 128ns ± 6% 119ns ± 1% -7.16% (p=0.000 n=10+10)
Gin_GithubAll 26.2µs ± 3% 25.3µs ± 1% -3.41% (p=0.000 n=10+10)
Gin_GPlusStatic 66.0ns ± 3% 58.2ns ± 2% -11.86% (p=0.000 n=10+10)
Gin_GPlusParam 91.2ns ± 3% 76.0ns ± 2% -16.70% (p=0.000 n=10+8)
Gin_GPlus2Params 117ns ± 5% 95ns ± 1% -19.17% (p=0.000 n=9+8)
Gin_GPlusAll 1.23µs ± 7% 1.09µs ± 2% -11.36% (p=0.000 n=10+10)
Gin_ParseStatic 62.4ns ± 5% 62.0ns ± 2% ~ (p=0.280 n=10+10)
Gin_ParseParam 70.2ns ± 6% 69.0ns ± 4% ~ (p=0.218 n=10+10)
Gin_Parse2Params 82.5ns ± 4% 83.0ns ± 2% ~ (p=0.271 n=10+10)
Gin_ParseAll 2.01µs ± 3% 1.94µs ± 2% -3.86% (p=0.008 n=10+10)
Gin_StaticAll 18.9µs ± 3% 18.4µs ± 1% -2.59% (p=0.005 n=10+10) |
Gin_Param5 112ns ± 6% 119ns ± 4% +6.74% (p=0.000 n=10+10)
Gin_Param20 282ns ± 3% 310ns ± 2% +10.09% (p=0.000 n=10+10) @rw-access @g1eny0ung any suggestion about this? |
hi @appleboy @rw-access @thinkerou I updated the code The latest benchmarks:
The latest benchstat report: name old time/op new time/op delta
Gin_Param 63.5ns ± 4% 61.3ns ± 1% -3.52% (p=0.001 n=10+10)
Gin_Param5 112ns ± 6% 116ns ± 1% +4.02% (p=0.002 n=10+9)
Gin_Param20 282ns ± 3% 305ns ± 1% +8.33% (p=0.000 n=10+9)
Gin_ParamWrite 119ns ± 5% 116ns ± 2% -2.97% (p=0.015 n=10+10)
Gin_GithubStatic 81.2ns ± 6% 80.3ns ± 0% ~ (p=0.172 n=10+10)
Gin_GithubParam 128ns ± 6% 136ns ± 1% +6.49% (p=0.000 n=10+10)
Gin_GithubAll 26.2µs ± 3% 28.1µs ± 0% +7.26% (p=0.000 n=10+10)
Gin_GPlusStatic 66.0ns ± 3% 61.4ns ± 0% -6.93% (p=0.000 n=10+10)
Gin_GPlusParam 91.2ns ± 3% 83.3ns ± 0% -8.65% (p=0.000 n=10+10)
Gin_GPlus2Params 117ns ± 5% 108ns ± 0% -8.00% (p=0.000 n=9+9)
Gin_GPlusAll 1.23µs ± 7% 1.19µs ± 0% ~ (p=0.464 n=10+10)
Gin_ParseStatic 62.4ns ± 5% 60.7ns ± 1% -2.75% (p=0.002 n=10+10)
Gin_ParseParam 70.2ns ± 6% 68.0ns ± 0% -3.08% (p=0.002 n=10+9)
Gin_Parse2Params 82.5ns ± 4% 85.8ns ± 0% +4.11% (p=0.000 n=10+9)
Gin_ParseAll 2.01µs ± 3% 1.99µs ± 0% ~ (p=0.466 n=10+10) |
hi erveryone,more balanced test reporting The latest benchmarks:
The latest benchstat report: name old time/op new time/op delta
Gin_Param 63.5ns ± 4% 61.4ns ± 5% -3.31% (p=0.019 n=10+10)
Gin_Param5 112ns ± 6% 115ns ± 3% +2.95% (p=0.012 n=10+10)
Gin_Param20 282ns ± 3% 304ns ± 2% +7.79% (p=0.000 n=10+10)
Gin_ParamWrite 119ns ± 5% 117ns ± 3% ~ (p=0.102 n=10+10)
Gin_GithubStatic 81.2ns ± 6% 78.4ns ± 2% -3.44% (p=0.009 n=10+10)
Gin_GithubParam 128ns ± 6% 133ns ± 1% +3.73% (p=0.001 n=10+10)
Gin_GithubAll 26.2µs ± 3% 27.1µs ± 0% +3.31% (p=0.004 n=10+10)
Gin_GPlusStatic 66.0ns ± 3% 62.7ns ± 2% -5.00% (p=0.000 n=10+10)
Gin_GPlusParam 91.2ns ± 3% 91.4ns ± 2% ~ (p=0.796 n=10+10)
Gin_GPlus2Params 117ns ± 5% 117ns ± 1% ~ (p=1.000 n=9+10)
Gin_GPlusAll 1.23µs ± 7% 1.19µs ± 1% ~ (p=0.565 n=10+10)
Gin_ParseStatic 62.4ns ± 5% 59.9ns ± 1% -3.92% (p=0.001 n=10+10)
Gin_ParseParam 70.2ns ± 6% 68.1ns ± 1% -2.88% (p=0.023 n=10+10)
Gin_Parse2Params 82.5ns ± 4% 83.8ns ± 1% ~ (p=0.089 n=10+10)
Gin_ParseAll 2.01µs ± 3% 1.96µs ± 1% -2.91% (p=0.003 n=10+10)
Gin_StaticAll 18.9µs ± 3% 19.1µs ± 0% ~ (p=0.156 n=10+9) |
LGTM, need @g1eny0ung @thinkerou @rw-access review and confirm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
merge and bump v1.7.3? thanks! @appleboy |
@thinkerou we can't release the v1.7.3 version since of the issue #2786 @qm012 I can reproduce the issue with the latest commit d4ca9a0 package main
import (
"fmt"
"github.com/gin-gonic/gin"
)
func handler01(c *gin.Context) {
fmt.Println(c.Params)
c.String(200, "ok01")
}
func handler02(c *gin.Context) {
fmt.Println(c.Params)
c.String(200, "ok02")
}
func main() {
g := gin.Default()
g.GET("/get/abc", handler01)
g.GET("/get/:param", handler02)
g.Run(":9090")
} result: /abc 200 ok02
/a 404
/ab 200 ok02
/xyz 200 ok02
/abcd 200 ok02 |
@qm012 @appleboy
Is it by design? |
@sdojjy No, the test cases have not been fully covered. Thank you for your feedback. |
this brought problems thus I am now frozen at #2878 |
(cherry picked from commit d4ca9a0)
(cherry picked from commit d4ca9a0)
content