-
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(tree): correctly expand the capacity of params #3502
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3502 +/- ##
=======================================
Coverage 99.20% 99.21%
=======================================
Files 42 42
Lines 3163 3175 +12
=======================================
+ Hits 3138 3150 +12
Misses 17 17
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hi, I noticed the
Not sure why it doesn't fail on older Go version or Ubuntu. I don't have a macOS machine to reproduce and it also doesn't look related to my changes. Would you mind having a look? |
Hello dear folks, hope you're having a great day ! |
Hello @appleboy 👋🏻, is there something we could do for this PR to land in |
@georgijd-form3 Can you add a performance report? |
Sure. Here are the results of running the following command: go test -bench=. -run '^Benchmark.*$' -count 10 and the comparison: $ benchstat master.txt fix-missing-params.txt
goos: darwin
goarch: arm64
pkg: github.com/gin-gonic/gin
│ master.txt │ fix-missing-params.txt │
│ sec/op │ sec/op vs base │
OneRoute-10 26.87n ± 1% 26.74n ± 0% ~ (p=0.059 n=10)
RecoveryMiddleware-10 31.45n ± 1% 31.77n ± 1% +1.02% (p=0.001 n=10)
LoggerMiddleware-10 789.4n ± 0% 791.7n ± 1% ~ (p=0.128 n=10)
ManyHandlers-10 821.5n ± 0% 821.2n ± 1% ~ (p=0.643 n=10)
5Params-10 62.91n ± 0% 65.23n ± 1% +3.69% (p=0.000 n=10)
OneRouteJSON-10 162.8n ± 0% 165.7n ± 1% +1.78% (p=0.000 n=10)
OneRouteHTML-10 608.1n ± 1% 614.4n ± 1% +1.02% (p=0.001 n=10)
OneRouteSet-10 150.0n ± 0% 149.8n ± 0% ~ (p=0.252 n=10)
OneRouteString-10 69.95n ± 0% 69.74n ± 0% -0.30% (p=0.002 n=10)
ManyRoutesFist-10 26.77n ± 0% 26.78n ± 0% ~ (p=0.643 n=10)
ManyRoutesLast-10 26.28n ± 0% 26.43n ± 0% +0.55% (p=0.012 n=10)
404-10 37.12n ± 1% 37.31n ± 1% +0.51% (p=0.028 n=10)
404Many-10 42.13n ± 1% 42.15n ± 0% ~ (p=0.516 n=10)
Github-10 1.164µ ± 1% 1.159µ ± 0% ~ (p=0.169 n=10)
ParallelGithub-10 728.0n ± 2% 725.6n ± 2% ~ (p=0.739 n=10)
ParallelGithubDefault-10 717.2n ± 2% 711.5n ± 2% ~ (p=1.000 n=10)
PathClean-10 877.3n ± 0% 879.0n ± 1% ~ (p=0.072 n=10)
PathCleanLong-10 2.694m ± 1% 2.678m ± 0% ~ (p=0.123 n=10)
ParseAccept-10 130.4n ± 0% 130.1n ± 1% ~ (p=0.538 n=10)
geomean 259.5n 260.3n +0.30%
│ master.txt │ fix-missing-params.txt │
│ B/op │ B/op vs base │
OneRoute-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
RecoveryMiddleware-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
LoggerMiddleware-10 268.0 ± 0% 268.0 ± 0% ~ (p=1.000 n=10) ¹
ManyHandlers-10 268.0 ± 0% 268.0 ± 0% ~ (p=1.000 n=10) ¹
5Params-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteJSON-10 48.00 ± 0% 48.00 ± 0% ~ (p=1.000 n=10) ¹
OneRouteHTML-10 256.0 ± 0% 256.0 ± 0% ~ (p=1.000 n=10) ¹
OneRouteSet-10 336.0 ± 0% 336.0 ± 0% ~ (p=1.000 n=10) ¹
OneRouteString-10 48.00 ± 0% 48.00 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesFist-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesLast-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404Many-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Github-10 1.056Ki ± 0% 1.056Ki ± 0% ~ (p=1.000 n=10) ¹
PathClean-10 112.0 ± 0% 112.0 ± 0% ~ (p=1.000 n=10) ¹
PathCleanLong-10 3.068Mi ± 0% 3.068Mi ± 0% ~ (p=0.372 n=10)
geomean ² -0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
│ master.txt │ fix-missing-params.txt │
│ allocs/op │ allocs/op vs base │
OneRoute-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
RecoveryMiddleware-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
LoggerMiddleware-10 8.000 ± 0% 8.000 ± 0% ~ (p=1.000 n=10) ¹
ManyHandlers-10 8.000 ± 0% 8.000 ± 0% ~ (p=1.000 n=10) ¹
5Params-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteJSON-10 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteHTML-10 9.000 ± 0% 9.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteSet-10 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹
OneRouteString-10 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesFist-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
ManyRoutesLast-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
404Many-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
Github-10 18.00 ± 0% 18.00 ± 0% ~ (p=1.000 n=10) ¹
PathClean-10 17.00 ± 0% 17.00 ± 0% ~ (p=1.000 n=10) ¹
PathCleanLong-10 4.683k ± 0% 4.683k ± 0% ~ (p=1.000 n=10) ¹
geomean ² +0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
|
@georgijd-form3 Thanks for your PR. :) |
Even though #2690 was fixed in #2755, the solution fixes the panic but doesn't actually expand the capacity of the params slice which means that the handler never gets those parameters in
context.Params
(as pointed out in #2690 (comment)). I have also added a router test to illustrate this - without the changes intree.go
it fails: