Skip to content

fix(tree): correctly expand the capacity of params#3502

Merged
appleboy merged 2 commits into
gin-gonic:masterfrom
georgijd-form3:fix-missing-params
Dec 7, 2023
Merged

fix(tree): correctly expand the capacity of params#3502
appleboy merged 2 commits into
gin-gonic:masterfrom
georgijd-form3:fix-missing-params

Conversation

@georgijd-form3
Copy link
Copy Markdown
Contributor

@georgijd-form3 georgijd-form3 commented Feb 16, 2023

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 in tree.go it fails:

=== RUN   TestRouteParamsNotEmpty
    routes_test.go:326:
                Error Trace:    /repos/gin/routes_test.go:326
                Error:          Not equal:
                                expected: "john"
                                actual  : ""

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -john
                                +
                Test:           TestRouteParamsNotEmpty
    routes_test.go:327:
                Error Trace:    /repos/gin/routes_test.go:327
                Error:          Not equal:
                                expected: "smith"
                                actual  : ""

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -smith
                                +
                Test:           TestRouteParamsNotEmpty
--- FAIL: TestRouteParamsNotEmpty (0.00s)

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 18, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (44d0dd7) 99.20% compared to head (bb52068) 99.21%.

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
Flag Coverage Δ
99.21% <100.00%> (+<0.01%) ⬆️
go-1.18 99.11% <100.00%> (+<0.01%) ⬆️
go-1.19 99.21% <100.00%> (+<0.01%) ⬆️
go-1.20 99.21% <100.00%> (+<0.01%) ⬆️
macos-latest 99.21% <100.00%> (+0.09%) ⬆️
ubuntu-latest 99.21% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@georgijd-form3
Copy link
Copy Markdown
Contributor Author

georgijd-form3 commented Feb 21, 2023

Hi,

I noticed the Run Tests / macos-latest @ Go 1.19 -tags "sonic avx" (pull_request) CI job failed. It appears that it expects fewer mallocs:

=== RUN   TestPathCleanMallocs
    path_test.go:85: 
        	Error Trace:	/Users/runner/work/gin/gin/path_test.go:85
        	Error:      	Not equal: 
        	            	expected: float64(1683)
        	            	actual  : int(0)
        	Test:       	TestPathCleanMallocs
--- FAIL: TestPathCleanMallocs (0.07s)

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?

@appleboy appleboy added the type/bug Found something you weren't expecting? Report it here! label Feb 24, 2023
@appleboy appleboy added this to the v1.10 milestone Feb 24, 2023
@cyryl-form3
Copy link
Copy Markdown

cyryl-form3 commented Oct 10, 2023

Hello dear folks, hope you're having a great day !
Is this PR something you would be willing to consider including/merging ? :)

@a-h a-h mentioned this pull request Nov 16, 2023
Closed
@tomas-deml-form3
Copy link
Copy Markdown

tomas-deml-form3 commented Dec 6, 2023

Hello @appleboy 👋🏻, is there something we could do for this PR to land in master? 🙏🏻
We maintain our own fork with the patch in production, and we think the fix is important for others as well.

@appleboy
Copy link
Copy Markdown
Member

appleboy commented Dec 6, 2023

@georgijd-form3 Can you add a performance report?

@georgijd-form3
Copy link
Copy Markdown
Contributor Author

georgijd-form3 commented Dec 6, 2023 •
edited
Loading

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

@appleboy appleboy merged commit 386d244 into gin-gonic:master Dec 7, 2023
@appleboy
Copy link
Copy Markdown
Member

appleboy commented Dec 7, 2023

@georgijd-form3 Thanks for your PR. :)

@appleboy appleboy mentioned this pull request Dec 7, 2023
Closed
@georgijd-form3 georgijd-form3 deleted the fix-missing-params branch December 7, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@appleboy appleboy appleboy approved these changes

Assignees

No one assigned

Labels

type/bug Found something you weren't expecting? Report it here!

Projects

None yet

Milestone

v1.10

Development

Successfully merging this pull request may close these issues.

4 participants

@georgijd-form3 @cyryl-form3 @tomas-deml-form3 @appleboy