Router: PoC stack based backtracking #1770
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1770 +/- ##
==========================================
- Coverage 89.74% 89.55% -0.20%
==========================================
Files 32 32
Lines 2672 2661 -11
==========================================
- Hits 2398 2383 -15
- Misses 175 179 +4
Partials 99 99
Continue to review full report at Codecov.
|
|
I've further improved the PoC and was able to remove almost all corner cases in the algorithm, which have been introduced to fix routing issues. |
|
@stffabi Thanks, very appreciated, There is quite some performance penality involved which should be reviewed: The above is from the automatic benchstat, so might be little off. But the numbers do indicate a performance impact that should be reviewed. |
|
@lammel You're very welcome. Yes I know there are some performance penalties. But as already mentioned it is in a PoC state and my first priority was to have a working and correct algorithm before introducing any optimizations. One question regarding the base of the benchstat of a commit. What's the base of the automatic benchstat of a commit? Is the I would be glad to invest some more time to improve the performance, if the echo community plans to have this merged. Unfortunately I had some bad experiences with other open source projects (not with echo 😄 ) lately, where I ended up with invested time and PRs that never got merged. |
stffabi
force-pushed
the
poc-router-stack-backtracking
branch
from
55a7287
to
3e817bc
Compare
|
I've improved the PoC, it now doesn't anymore store the backtrack information on the heap but on the goroutine's stack. As a consequence the maximum level of nested backtracking needs to be known beforehand during compile-time. Currently as this is PoC, the I've started to experiment around with different values for the max backtracking depth. First started with 1 (which would be identical with the current implementation) and then I've successively increased it. Please note in order that all benchmark tests run and don't panic we need at least a max depth of 2. Here are the results (benchmarks done on my local machine), delta is always compared to old (932976d) As one can see, the new algorithm with a max depth of 2 outperforms the current version in almost any benchmark. Furthermore one can see that by increasing the max depth the performance begins to degrade, especially for the So what could we conclude from this: One has to balance between the max depth of backtracking echo could support and the performance penalty one is willing to pay for it. I currently see no options to further improve this. First we have to store the backtracking information somewhere, The only options I could see, but those options could make the code harder to maintain, are the following:
This would allow echo to work with the best performance depending on the nested backtracking depth, that the user has registered on the router. |
Your work will be very valuable even if this PoC PR will not be merged. We can't do any promises on that, but even then this work will provide the foundation for other PRs to improve on that concepts! There are some other ideas around that could be efficient too (both in terms of maintainability and performance). What we really want to avoid is having multiple implementations for the router or duplicate router functions. |
|
Another option that could possibly bring improvements would be to use tail recursion, but this would highly depend on the optimizations the go compiler does and might change with go versions. Which is another decision one would have to make if echo wants to go that path.
I understand that, I have no problem if it's not my PR that gets merged 😄, as long as the issue gets fixed in a timely manner and I could help along that path. The problem in the past with other projects was that on the first creation of the PR I got feedback. I adjusted the PR with the demanded changes and then got never any feedback and the issues still persists in the other projects, now for more than 2 years. If i could ask, what are those ideas? I would love to hear about them and possibly participate on that discussions. Unfortunately I couldn't found any public discussion about those. Maybe other Draft PRs for those ideas could be created to compare all of them and to compare the pros and cons of every idea. |
Might be an option, at least we would participate in advancements of the go optimizer. But I'm also not sure this is the right way to go.
Seems like recently the merges are pretty reasonable. Hope we can say that for this PR then too ;-)
We planned to open a public discussion about ideas for the router.
On of the topics for simplifcation of the router.Find function tackled by the PR actually. |
|
I think @stffabi version is quite good fix. Performance hit on backtracking can not be avoided as by algoritm backtracking is done until suitable node is found up in tree to the right or we are back at root node and there are nowhere else to go. @stffabi version is quite nice as it simplifies current implementation quite a bit and makes it more approachable. If that depth calculation is moved to I tried to implement same algorithm but without stack (as I thought it would definitely mean allocating) and result are similar - misses (backtracking) is always performance hit. My solution https://github.com/aldas/echo/blob/issue-1754_router_backtracking/router.go#L552 And comparison to
x@x:~/code/echo$ benchstat old.out stffabi.out
name old time/op new time/op delta
RouterStaticRoutes-6 16.0µs ± 1% 15.1µs ± 3% -5.12% (p=0.000 n=20+19)
RouterStaticRoutesMisses-6 515ns ± 1% 615ns ± 3% +19.48% (p=0.000 n=20+19)
RouterGitHubAPI-6 30.1µs ± 3% 33.0µs ±15% +9.68% (p=0.000 n=20+20)
RouterGitHubAPIMisses-6 621ns ± 0% 678ns ± 5% +9.15% (p=0.000 n=16+20)
RouterParseAPI-6 1.61µs ± 2% 1.55µs ± 3% -3.64% (p=0.000 n=20+18)
RouterParseAPIMisses-6 349ns ± 1% 404ns ± 1% +15.81% (p=0.000 n=20+17)
RouterGooglePlusAPI-6 1.04µs ± 0% 1.01µs ± 3% -3.36% (p=0.000 n=15+20)
RouterGooglePlusAPIMisses-6 516ns ± 1% 547ns ± 4% +6.00% (p=0.000 n=19+20)
RouterParamsAndAnyAPI-6 2.44µs ± 1% 2.34µs ± 6% -4.31% (p=0.000 n=20+20)
x@x:~/code/echo$ benchstat new.out stfabi.out
name old time/op new time/op delta
RouterStaticRoutes-6 14.9µs ± 1% 15.1µs ± 3% +1.77% (p=0.000 n=18+19)
RouterStaticRoutesMisses-6 585ns ± 2% 615ns ± 3% +5.20% (p=0.000 n=20+19)
RouterGitHubAPI-6 31.2µs ± 1% 33.0µs ±15% +5.69% (p=0.033 n=20+20)
RouterGitHubAPIMisses-6 763ns ± 1% 678ns ± 5% -11.15% (p=0.000 n=20+20)
RouterParseAPI-6 1.38µs ± 3% 1.55µs ± 3% +12.23% (p=0.000 n=20+18)
RouterParseAPIMisses-6 394ns ± 1% 404ns ± 1% +2.58% (p=0.000 n=16+17)
RouterGooglePlusAPI-6 887ns ± 2% 1009ns ± 3% +13.69% (p=0.000 n=19+20)
RouterGooglePlusAPIMisses-6 616ns ± 0% 547ns ± 4% -11.08% (p=0.000 n=14+20)
RouterParamsAndAnyAPI-6 2.43µs ± 2% 2.34µs ± 6% -3.89% (p=0.000 n=20+20)
x@x:~/code/echo$ benchstat old.out new.out
name old time/op new time/op delta
RouterStaticRoutes-6 16.0µs ± 1% 14.9µs ± 1% -6.77% (p=0.000 n=20+18)
RouterStaticRoutesMisses-6 515ns ± 1% 585ns ± 2% +13.58% (p=0.000 n=20+20)
RouterGitHubAPI-6 30.1µs ± 3% 31.2µs ± 1% +3.78% (p=0.000 n=20+20)
RouterGitHubAPIMisses-6 621ns ± 0% 763ns ± 1% +22.86% (p=0.000 n=16+20)
RouterParseAPI-6 1.61µs ± 2% 1.38µs ± 3% -14.14% (p=0.000 n=20+20)
RouterParseAPIMisses-6 349ns ± 1% 394ns ± 1% +12.90% (p=0.000 n=20+16)
RouterGooglePlusAPI-6 1.04µs ± 0% 0.89µs ± 2% -14.99% (p=0.000 n=15+19)
RouterGooglePlusAPIMisses-6 516ns ± 1% 616ns ± 0% +19.20% (p=0.000 n=19+14)
RouterParamsAndAnyAPI-6 2.44µs ± 1% 2.43µs ± 2% -0.43% (p=0.014 n=20+20)also I think all implementation have atm 1 failing testcase. When backtracking from func TestRouterBacktrackingFromParamAny2(t *testing.T) {
e := New()
r := e.router
r.Add(http.MethodGet, "/*", handlerHelper("case", 1))
r.Add(http.MethodGet, "/:name", handlerHelper("case", 2))
r.Add(http.MethodGet, "/:name/lastname", handlerHelper("case", 3))
c := e.NewContext(nil, nil).(*context)
r.Find(http.MethodGet, "/firstname/", c) // trailing slash confuses param kind matching algorithm
c.handler(c)
assert.Equal(t, "/:name", c.Get("path")) // FIXME: currently matches `/*` which is low priority than `/:name` and therefore should not be matched
assert.Equal(t, "firstname/", c.Param("*"))
r.Find(http.MethodGet, "/firstname/test", c)
c.handler(c)
assert.Equal(t, "/:name", c.Get("path")) // FIXME: currently matches `/*`
assert.Equal(t, "firstname/test", c.Param("*"))
} |
|
also it would probably help if instead of state [backTrackingDepth]struct {
nk kind
nn *node
searchLen int
np int
}and in pop() search = path[last.searchLen:]
searchLen = last.searchLenPatch: Result: x@x:~/code/echo$ benchstat old.out stfabi_int.out
name old time/op new time/op delta
RouterStaticRoutes-6 16.0µs ± 1% 16.1µs ± 6% ~ (p=0.147 n=20+20)
RouterStaticRoutesMisses-6 515ns ± 1% 603ns ± 1% +17.09% (p=0.000 n=20+18)
RouterGitHubAPI-6 30.1µs ± 3% 29.2µs ± 1% -2.94% (p=0.000 n=20+20)
RouterGitHubAPIMisses-6 621ns ± 0% 681ns ± 5% +9.68% (p=0.000 n=16+20)
RouterParseAPI-6 1.61µs ± 2% 1.63µs ± 2% +1.61% (p=0.000 n=20+18)
RouterParseAPIMisses-6 349ns ± 1% 392ns ± 0% +12.32% (p=0.000 n=20+18)
RouterGooglePlusAPI-6 1.04µs ± 0% 1.05µs ± 2% +0.96% (p=0.001 n=15+20)
RouterGooglePlusAPIMisses-6 516ns ± 1% 526ns ± 1% +1.85% (p=0.000 n=19+19)
RouterParamsAndAnyAPI-6 2.44µs ± 1% 2.43µs ± 0% -0.56% (p=0.000 n=20+19) |
|
@lammel I think we should accept this idea as a solution. Whatever the future holds for router this PR helps with backtracking and simplifies current solution. This will not prevent different paths and ideas for future. Only noticeable change is for misses. As current implementation does not handle backtracking anyway in depth - I would assume that current users do not have routing trees that would be affected much. For those 404 cases - these are off the happy path anyway and are not that critical for execution. Also - for example going from 3 microsecond (µs) to 4 microsecond for thing that is executed once per request should be treated differently that something that is executed multiple times per request (some changes seems to be even on nanosecond land). At least the context where the change happens should also be considered and why it happens - we support one feature (backtracking) fully now. Just looking at plain -+% should not be the only criteria Lets get this PR cleaned up and merged and iterate on this in future if needed or when some finds he/she can do better. |
|
Based on the benchmarks I've seen this looks promising anyway, especially cleaning up the "special cases" in the Find logic. So I'm definitly in favour of cleaning up this PR an get it merged! We still need to define the behaviur of the router for back tracking and add appropriate tests with a comment (even if it turns out to be the wrong decision in the future). I'm running out if time this week to check on that probably. |
I think the current benchmarks are a little bit misleading, because the current benchmarks time reported as Wouldn't something like this be more accurate in // Find routes
c := e.pool.Get().(*context)
for i := 0; i < b.N; i++ {
route := routesToFind[i%len(routesToFind)]
r.Find(route.Method, route.Path, c)
}
e.pool.Put(c)First we would have not included the time to get the context from the pool (which is not really part of the router) and we would have a more accurate Benchmarking with this adjustments even shows much smaller absolute performance regression per request, on my local machine in the range of 1-2 nanosecond per request. |
|
@stffabi has quite good test cases for backtracking - I found 2 problems with them in my implementation and that 1 problem that all implementations have after thinking/reviewing cases. We can extend test suite with own cases that we come up to. I would even like to add PR on top of it taken from my own branch just for to convert test cases to table driven (already done in my branch) as it would help debugging cases so much - adding breakpoints and only executing certain case is such a time saver with breakpoints. |
@aldas yeah that's a very nice idea, It also popped up on my list to give it try. |
|
If @stffabi allowed for maintainers then @aldas can push right into this branch, you could work together right on this PR. Otherwise we need to first merge this one and use the changes by @aldas in a follow-up PR. |
|
I think it would be easier for @stffabi (and understand what was changed when looking in history) if we finish this PR without me touching tests and renaming stuff and I'll add my PR after this is merged. These are just minor refinements are better of with their own history. If @stffabi is willing to finish implementing/moving depth calculation out of |
|
Ok. If it's easier that way then may @stffabi could move the depth calculation to |
@aldas Yes of course I would be happy to do that, I should be able to do this during the next few days. @aldas, @lammel could we first write down what our final decisions are?
|
Unfortunately that's not possible this way. If we want to put the backtracking information onto the Goroutine`s stack the max level we can support must be a compile time constant. |
|
Ok, I did not know that it has to be set compile time. well, we can not have always ideal solutions. I would suggest then:
|
|
Yes it's too bad we can't assign that dynamically. I still have some concerns regarding the |
|
Panicing would be fatal to current users that 'rely' on current 'silent' behavior and are ok with 404. I'd add note in echo documentation about that behavior and maybe todo/fixme in this repo. There are actually other things/problems that route.Insert also ignores - ala settings different param names for different routes with same prefix. If you have yet (over the days/weeks/months/years) noticed getting 404 so far then you do care about that route and handler. Panic would mean that you would not get to start server and to do thing that you care about. I have mentioned in somewhere that actually in future (v5 maybe) when adding a route we should return an error. This would be one thing to return error |
|
I see your argument, but if we want to argument that way we have to consider a lot more. We then can't just return 404 in the case we exceed the max backtracking depth of 20 or 30. If we see the current behaviour as a feature, then we would have to introduce some kind of feature flag (maybe an ENV variable like the Go-Team does with GODEBUG?) to either select the old or the new algorithm. Or we have to wait for |
|
We definitely don't want a feature flags or environment variables for the router. This will just make testing and support harder. So for v4 we can define the behavior and go with a minimal solution (like this PR to fix unexpected not found). Or we flag this PR also for v5 and can do breaking changes. Still with the current requirement to define a compile time constant for backtracking depth we are vary limited it seems. |
|
Well I think there are some philosophy questions which the maintainers need to clarify for themselves. Those are clearly out of my scope. As for my part on this issue, you have a draft PR and are allowed to further improve or abandon it. |
|
I think we are little bit overly dramatic about limitations of this solution. But do not worry @stffabi stack-clousure solution can be turned into algorithmic backtracking. I have already implemented/proved https://github.com/aldas/echo/blob/issue-1754_router_backtracking/router.go#L552 so when rules from upwards traversal are as stated in #1754 (comment) we can calculate those in that |
|
ok, I unable to push to @stffabi branch (probably doing something wrong) and i'll just post patch here (renamed to .txt as github does not allow uploading .patch files) Complete change: Briefly main change is 'pop' method: backtrackNode := func(fromKind kind) (nodeKind kind, valid bool) {
previous := cn
cn = previous.parent
valid = cn != nil
// next node type by priority
switch previous.kind {
case akind:
nodeKind = skind
default:
nodeKind = previous.kind + 1
}
if fromKind == skind {
// when backtracking is done from static kind block we did not change search so nothing to restore
return
}
if previous.kind == skind {
searchIndex -= len(previous.prefix)
search = path[searchIndex:]
} else {
n--
searchIndex -= len(pvalues[n])
search = path[searchIndex:]
}
return
}and benchmark from my pc with current Echo implementation go test -run="-" -bench="BenchmarkRouter.*" -count 20 > ~/code/echo/stffabi_nostack.out
...
x@x:~/code/echo$ benchstat old.out stffabi_nostack.out
name old time/op new time/op delta
RouterStaticRoutes-6 16.0µs ± 1% 13.8µs ± 1% -13.52% (p=0.000 n=20+18)
RouterStaticRoutesMisses-6 515ns ± 1% 543ns ± 1% +5.44% (p=0.000 n=20+20)
RouterGitHubAPI-6 30.1µs ± 3% 27.1µs ± 2% -9.79% (p=0.000 n=20+19)
RouterGitHubAPIMisses-6 621ns ± 0% 642ns ± 0% +3.32% (p=0.000 n=16+18)
RouterParseAPI-6 1.61µs ± 2% 1.43µs ± 4% -11.12% (p=0.000 n=20+20)
RouterParseAPIMisses-6 349ns ± 1% 346ns ± 1% -1.01% (p=0.000 n=20+18)
RouterGooglePlusAPI-6 1.04µs ± 0% 0.93µs ± 1% -11.32% (p=0.000 n=15+18)
RouterGooglePlusAPIMisses-6 516ns ± 1% 503ns ± 0% -2.67% (p=0.000 n=19+18)
RouterParamsAndAnyAPI-6 2.44µs ± 1% 2.15µs ± 3% -11.89% (p=0.000 n=20+20) |
Sorry, just wanted to help and state my* personal views* on what issues may arise with decisions like a hardcoded stacksize. I see no philosophical question though, just some discussions that need to be settled.
We are aiming for option number 1 👍🏼 |
|
@stffabi if you would apply that patch I think we are done or very close to be ready to merge. |
PoC regarding issue #1754