-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat(router): implement router #10
Conversation
考虑一下 edge case:emoji 🍆 in path |
|
||
// AddExecutor adds executor to the router node. | ||
// A router can hold many executors, but there is only one executor | ||
// is selected for a match. |
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.
remove is
// FindStringRouter find a router by first char. | ||
func (p *Progeny) FindStringRouter(char byte) Router { | ||
length := len(p.StringRouters) | ||
if length <= 3 { |
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.
3 is not a good heuristic. maybe try 20-50? linear search is not that slow, you got space locality and line cache.
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.
I have tested it with:
package main
import (
"fmt"
"sort"
"time"
)
type CharRouter struct {
Char byte
Pointer *int
}
type Progeny struct {
StringRouters []CharRouter
}
func (p *Progeny) FindStringRouter(char byte) bool {
length := len(p.StringRouters)
if length <= 0 {
return false
}
index := sort.Search(len(p.StringRouters), func(i int) bool {
return char <= p.StringRouters[i].Char
})
return index < length && char == p.StringRouters[index].Char
}
func (p *Progeny) Find(char byte) bool {
for _, cr := range p.StringRouters {
if cr.Char == char {
return true
}
}
return false
}
func main() {
p := &Progeny{
StringRouters: make([]CharRouter, 0, 256),
}
for i := 0; i < 256; i++ {
p.StringRouters = append(p.StringRouters, CharRouter{byte(i), nil})
if i >= 0 {
Test(p, 1000000)
}
}
}
func Test(p *Progeny, count int) {
length := len(p.StringRouters)
counter := 0
totalBSR := int64(0)
totalLSR := int64(0)
for i := 0; i < length; i++ {
bsr := TestBS(p, byte(i), count)
lsr := TestLS(p, byte(i), count)
if bsr < lsr {
counter++
}
totalBSR += bsr
totalLSR += lsr
}
fmt.Printf("Len: %d \t BS:%d, %.2f%% \t Total: BS(%d), LS(%d)\n", length, counter, float64(counter)*100/float64(length), totalBSR, totalLSR)
}
func TestBS(p *Progeny, char byte, count int) int64 {
start := time.Now().UnixNano()
for i := 0; i < count; i++ {
p.FindStringRouter(char)
}
return time.Now().UnixNano() - start
}
func TestLS(p *Progeny, char byte, count int) int64 {
start := time.Now().UnixNano()
for i := 0; i < count; i++ {
p.Find(char)
}
return time.Now().UnixNano() - start
}
The result is:
...
Len: 101 BS:44, 43.56% Total: BS(2081710163), LS(1990178100)
Len: 102 BS:46, 45.10% Total: BS(2103198775), LS(2023960297)
Len: 103 BS:46, 44.66% Total: BS(2118603745), LS(2056275622)
Len: 104 BS:47, 45.19% Total: BS(2148520686), LS(2104218474)
Len: 105 BS:48, 45.71% Total: BS(2180732642), LS(2159296633)
Len: 106 BS:49, 46.23% Total: BS(2208165772), LS(2206370126)
Len: 107 BS:50, 46.73% Total: BS(2272162642), LS(2313611472)
Len: 108 BS:51, 47.22% Total: BS(2309499809), LS(2336858570)
Len: 109 BS:51, 46.79% Total: BS(2306542632), LS(2342357262)
Len: 110 BS:54, 49.09% Total: BS(2322731078), LS(2388230653)
Len: 111 BS:53, 47.75% Total: BS(2315820134), LS(2392378395)
Len: 112 BS:54, 48.21% Total: BS(2331587726), LS(2437614218)
Len: 113 BS:55, 48.67% Total: BS(2413778245), LS(2549104883)
Len: 114 BS:57, 50.00% Total: BS(2457508030), LS(2589879197)
Len: 115 BS:57, 49.57% Total: BS(2446028708), LS(2591940396)
Len: 116 BS:60, 51.72% Total: BS(2517156756), LS(2704493175)
...
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.
It seems that 106 is the boundary.
index := 0 | ||
if length > 0 { | ||
index = sort.Search(length, func(i int) bool { | ||
return c < p.StringRouters[i].Char |
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.
where's the discrepancy between
c < p.StringRouters[i].Char
and
char <= p.StringRouters[i].Char
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.
An array:
[a, b, c, d, e, f]
if c is 'd', c < p.StringRouters[i].Char
takes index 4, char <= p.StringRouters[i].Char
is 3.
if index >= length { | ||
p.StringRouters = append(p.StringRouters, cr) | ||
} else { | ||
p.StringRouters = append(p.StringRouters[:index+1], p.StringRouters[index:]...) |
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.
why flip?
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.
It does not flip. It just moves [index:end] to [index+1:end+1]. Then insert cr
to [index].
"reflect" | ||
"testing" | ||
) | ||
|
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.
this part needs to be more heavily tested:
- unions like:
{name:(aaa|bbb)}
- regex validity pre-checking: invalid regex like:
{name:(a}
should not pass - overlaps checking:
/path/{name:[a-z]*}
and/path/ddy
are overlapping
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.
Also does it support inner catch group for url? i hope not:
{name:some-format-(\d+)-caught}
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.
- All valid regular expression is allowed here.
- The path
{name:(a}
will get an error afterParse()
- Priority: String Node > Regexp Node > Path Node. So
/path/ddy
is prior to/path/{name:[a-z]*}
.
It supports {name:some-format-(\d+)-caught}
, but its value won't be extracted.
what is the behavior between canonical url and non-canonical urls regarding trailing slash?
|
@jimexist Supported. Emoji is same as other chars. |
The two paths is different for router. But we can handle trailing slash in framework. |
progeny 这个名字比较深奥,可以用一个简单的名字,比如 children? |
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.
approve to move things along
有很多error可以从 |
// Handler contains middlewares and executor. | ||
type Handler struct { | ||
Middlewares []Middleware | ||
Executor Executor |
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.
这里为什么不直接是Executors
?
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.
+1
return &FullMatchRegexpNode{ | ||
Key: seg.Keys[0], | ||
}, nil | ||
} else { |
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.
这个else没什么意义了,上面都return了
+1 |
String RouterKind = "String" | ||
// Regexp means the router has a regular expression. | ||
Regexp RouterKind = "Regexp" | ||
// Path means the router matches the rest. Path router only can |
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.
the rest (of what) ?
} | ||
|
||
// Merge merges r to the current router. The type of r should be same | ||
// as the current one or it panics. |
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.
没有panic啊?
有些错误处理测试没有覆盖到。
|
LGTM. Discussed offline, this PR is ready to go. |
Implement regexp router.
Performance: