Skip to content

Commit

Permalink
Merge pull request #61 from yookoala/fix/regex-reuse-issue
Browse files Browse the repository at this point in the history 
Improve performance by using copy instead of recompile
  • Loading branch information
@yookoala
yookoala authored Feb 5, 2021
2 parents e5eda92 + a512850 commit bd30ad8
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions session.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ type FileSystemRouter struct {
// DOCUMENT_ROOT
//
func (fs *FileSystemRouter) Router() Middleware {
pathinfoRe := regexp.MustCompile(`^(.+\.php)(/?.+)$`)
return func(inner SessionHandler) SessionHandler {
return func(client Client, req *Request) (*ResponsePipe, error) {

Expand All @@ -207,8 +208,7 @@ func (fs *FileSystemRouter) Router() Middleware {
fastcgiScriptName := r.URL.Path

var fastcgiPathInfo string
pathinfoRe := regexp.MustCompile(`^(.+\.php)(/?.+)$`)
if matches := pathinfoRe.FindStringSubmatch(fastcgiScriptName); len(matches) > 0 {
if matches := pathinfoRe.Copy().FindStringSubmatch(fastcgiScriptName); len(matches) > 0 {
fastcgiScriptName, fastcgiPathInfo = matches[1], matches[2]
}

Expand Down

3 comments on commit bd30ad8

@joonas-fi
Copy link

Choose a reason for hiding this comment

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

@yookoala: sorry to nitpick again, but Copy() of the regexp is not necessary (it's marked as deprecated), as using the regexp doesn't modify its internal state. They were designed to be compiled and used over and over again :) Only exception would be if you modified the regexp's settings per request , but AFAIK calling Longest() is the only setting you can modify after compiling and that's not done here.

@joonas-fi
Copy link

Choose a reason for hiding this comment

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

If you're fixing that, there's another place where regexp is compiled for every request:

gofast/session.go

Line 338 in b30b52d

pathinfoRe := regexp.MustCompile(`^(.+\.php)(/?.+)$`)

@yookoala
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm currently rethinking the entire routing process.

I didn't want to make it too complicated in my first go. But seems more people is using this than I expected. So I'd probably spend some more times on it.

Please sign in to comment.