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.

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

Update the code logic for latestNode in tree.go #2897

Merged
merged 7 commits into from
Oct 23, 2021

Conversation

zhuxi0511
Copy link
Contributor

@zhuxi0511 zhuxi0511 commented Oct 9, 2021

This PR is a new code solution for #2820 and it also solve the issues #2894 #2878

the old latestNode logic should be modified to save a list of node. Then the error match for fixed path can be roll back to the right param node and right path.

the benchmark result:

master:https://app.travis-ci.com/github/zhuxi0511/go-http-routing-benchmark/builds/239484250
this PR:https://app.travis-ci.com/github/zhuxi0511/go-http-routing-benchmark/builds/239484068

name              old time/op    new time/op    delta
Gin_Param           62.8ns ± 1%    64.8ns ± 3%  +3.13%  (p=0.004 n=10+10)
Gin_Param5           118ns ± 3%     115ns ± 3%  -2.96%  (p=0.000 n=10+10)
Gin_Param20          310ns ± 0%     294ns ± 1%  -5.06%  (p=0.000 n=10+9)
Gin_ParamWrite       118ns ± 1%     122ns ± 3%  +3.68%  (p=0.000 n=8+9)
Gin_GithubStatic    75.2ns ± 1%    76.4ns ± 3%    ~     (p=1.000 n=8+10)
Gin_GithubParam      134ns ± 1%     133ns ± 2%    ~     (p=0.824 n=9+10)
Gin_GithubAll       26.0µs ± 0%    26.8µs ± 1%  +2.94%  (p=0.000 n=9+10)
Gin_GPlusStatic     59.0ns ± 0%    59.0ns ± 0%    ~     (p=0.445 n=10+10)
Gin_GPlusParam      84.4ns ± 3%    80.1ns ± 0%  -5.07%  (p=0.000 n=10+10)
Gin_GPlus2Params     109ns ± 2%     102ns ± 1%  -6.13%  (p=0.000 n=10+10)
Gin_GPlusAll        1.14µs ± 0%    1.12µs ± 0%  -1.87%  (p=0.000 n=8+10)
Gin_ParseStatic     59.8ns ± 6%    63.2ns ± 3%  +5.71%  (p=0.000 n=10+10)
Gin_ParseParam      68.9ns ± 2%    71.9ns ± 1%  +4.30%  (p=0.000 n=9+10)
Gin_Parse2Params    85.1ns ± 0%    85.1ns ± 1%    ~     (p=0.138 n=10+10)
Gin_ParseAll        1.93µs ± 0%    2.00µs ± 1%  +3.40%  (p=0.000 n=10+10)
Gin_StaticAll       18.2µs ± 0%    18.0µs ± 0%  -0.98%  (p=0.000 n=10+9)

I am a new goer, so some code may not good enough. Thank you for your help and review.

This PR has 3 similar code section for roll back skippedNode. I just keep this code for simple logic to get right route guarantee. It should be optimized in next PR, Thanks!

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #2897 (7fc975f) into master (5929d52) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2897   +/-   ##
=======================================
  Coverage   98.72%   98.73%           
=======================================
  Files          41       41           
  Lines        3066     3089   +23     
=======================================
+ Hits         3027     3050   +23     
  Misses         27       27           
  Partials       12       12           
Flag Coverage Δ
go-1.13 98.73% <100.00%> (+<0.01%) ⬆️
go-1.14 98.57% <100.00%> (+0.01%) ⬆️
go-1.15 98.57% <100.00%> (+0.01%) ⬆️
go-1.16 98.57% <100.00%> (+0.01%) ⬆️
go-1.17 98.47% <100.00%> (+0.01%) ⬆️
macos-latest 98.73% <100.00%> (+<0.01%) ⬆️
nomsgpack 98.71% <100.00%> (+<0.01%) ⬆️
ubuntu-latest 98.73% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
context.go 97.82% <100.00%> (+<0.01%) ⬆️
gin.go 99.11% <100.00%> (+0.01%) ⬆️
tree.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5929d52...7fc975f. Read the comment docs.

@Bisstocuz
Copy link
Contributor

You need to check out the Codecov and refine the unit tests. 😀

@zhuxi0511
Copy link
Contributor Author

zhuxi0511 commented Oct 15, 2021

You need to check out the Codecov and refine the unit tests. 😀

Already added more test for codecov test, THX

Copy link
Member

@appleboy appleboy left a comment

Choose a reason for hiding this comment

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

LGTM

@Bisstocuz
Copy link
Contributor

Does this PR can fix #2843 #2847 ? 😁
If can, I think maintainers will published a new update after this PR merged.

@zhuxi0511
Copy link
Contributor Author

Does this PR can fix #2843 #2847 ? 😁 If can, I think maintainers will published a new update after this PR merged.

I have tried the case in #2843. But regret for that this PR cannot fix this issue. It may need other PR to fix. @Bisstocuz

@appleboy
Copy link
Member

#2847 PR fixed the #2843 issue

@appleboy
Copy link
Member

@thinkerou need your feedbacks.

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm

@ibraheemdev
Copy link
Contributor

I think there are still some bugs. The following works:

tree := &node{}
tree.addRoute("/get/:param", fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))

But if you change the param name of one of them, it doesn't

tree := &node{}
tree.addRoute("/get/:a" fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))
panic: ':param' in new path '/get/:param/foo' conflicts with existing wildcard ':a' in existing prefix '/get/:a'

@zhuxi0511
Copy link
Contributor Author

I think there are still some bugs. The following works:

tree := &node{}
tree.addRoute("/get/:param", fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))

But if you change the param name of one of them, it doesn't

tree := &node{}
tree.addRoute("/get/:a" fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))
panic: ':param' in new path '/get/:param/foo' conflicts with existing wildcard ':a' in existing prefix '/get/:a'

@ibraheemdev this is not a bug. It's a restriction for param route in my mind.
And this panic happened in the tree building progress, the earlier version of gin should be same behavior. You can have a try.

@ibraheemdev
Copy link
Contributor

I guess it's not a bug, but it's something I would expect to be supported. Would the current backtracking logic handle this?

@zhuxi0511
Copy link
Contributor Author

I guess it's not a bug, but it's something I would expect to be supported. Would the current backtracking logic handle this?

The current logic can only process the tree path match function, and this all after tree building progress.
And that panic happened in the tree building progress. They are in two different stages. So you may need a new PR~

@ibraheemdev
Copy link
Contributor

Yes, but if the backtracking logic can handle those routes, then only the insert function has to be changed.

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants