-
Notifications
You must be signed in to change notification settings - Fork 619
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
Add new & improved glob matcher #457
Add new & improved glob matcher #457
Conversation
f65d00b
to
48dbe18
Compare
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.
You can cache the compiled pattern in the Route
object and update the addRoute
method in the Table
https://github.com/fabiolb/fabio/blob/master/route/table.go#L137-L173
Also, If this glob library is backwards-compatible with https://github.com/ryanuber/go-glob then I would suggest to just replace it instead of adding another one. Can you check?
Hey @magiconair, I've implemented the fixes you've requested. |
route/matcher_test.go
Outdated
func getRoute(path string) *Route { |
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'd prefer if you compile the glob matcher in the test since this keeps the changeset smaller, e.g.
for _, tt := range tests {
t.Run(tt.uri, func(t *testing.T) {
tt.route.Matcher = glob.MustCompile(tt.route.path)
...
})
}
route/route.go
Outdated
@@ -36,6 +37,9 @@ type Route struct { | |||
// total contains the total number of requests for this route. | |||
// Used by the RRPicker | |||
total uint64 | |||
|
|||
// Matcher represents compiled pattern. | |||
Matcher glob.Glob |
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 think Glob
is a better name than Matcher
.
route/table.go
Outdated
@@ -14,6 +14,7 @@ import ( | |||
|
|||
"github.com/fabiolb/fabio/metrics" | |||
"github.com/ryanuber/go-glob" | |||
glob2 "github.com/gobwas/glob" |
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.
Please name this glob
and also remove the github.com/ryanuber/go-glob
library in a separate commit.
route/table.go
Outdated
@@ -153,7 +154,7 @@ func (t Table) addRoute(d *RouteDef) error { | |||
switch { | |||
// add new host | |||
case t[host] == nil: | |||
r := &Route{Host: host, Path: path} | |||
r := &Route{Host: host, Path: path, Matcher: glob2.MustCompile(path)} |
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.
What happens if this is an invalid pattern? Does it panic
? You probably need to catch the error and return it.
@@ -290,7 +298,8 @@ func (t Table) matchingHosts(req *http.Request) (hosts []string) { | |||
host := normalizeHost(req.Host, req.TLS != nil) | |||
for pattern := range t { | |||
normpat := normalizeHost(pattern, req.TLS != nil) | |||
if glob.Glob(normpat, host) { |
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'm not sure why you used the glob matcher here in the first place, can a host contain wildcards ?
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.
Good question. I'll check.
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.
Yes, you can have a route for *.example.com/foo
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 wrote a small benchmark to verify performance. gobwas
is fast but only when the expression is pre-compiled. It is only 1.4 ms but this sits in the hot-path. However, there is no easy way to refactor this since Table
is just a map[string]Routes
. Might be a good time to finally change this to a struct. I'll have a look.
$ go test -bench .
goos: darwin
goarch: amd64
pkg: globbench
BenchmarkGlob/ryanuber-8 20000000 91.4 ns/op
BenchmarkGlob/gobwas-8 1000000 1385 ns/op
BenchmarkGlob/gobwas-precompile-8 200000000 7.29 ns/op
PASS
ok globbench 5.537s
package main
import "testing"
import ryanuber "github.com/ryanuber/go-glob"
import gobwas "github.com/gobwas/glob"
var n int
func BenchmarkGlob(b *testing.B) {
b.Run("ryanuber", func(b *testing.B) {
for i := 0; i < b.N; i++ {
if ryanuber.Glob("*.example.com", "foo.example.com") {
n++
}
}
})
b.Run("gobwas", func(b *testing.B) {
for i := 0; i < b.N; i++ {
m := gobwas.MustCompile("*.example.com")
if m.Match("foo.example.com") {
n++
}
}
})
b.Run("gobwas-precompile", func(b *testing.B) {
m := gobwas.MustCompile("*.example.com")
for i := 0; i < b.N; i++ {
if m.Match("foo.example.com") {
n++
}
}
})
b.Log(n)
}
…hub.com/ryanuber/go-glob
I've added a new matcher using
gobwas/glob
side-by-side to the existing glob matcherThis might hinder performance due to the fact that the matcher is compiling the pattern each time.
I was thinking of using a cache for the compiled matchers (something like
patrickmn/go-cache
).@magiconair what do you think?
#452