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.

Sign up for GitHub

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

Bug 1937972: router/template: Cache compiled regular expressions #268

Merged
merged 2 commits into from
Mar 12, 2021
Merged

Bug 1937972: router/template: Cache compiled regular expressions #268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19b090e
optimisation: cache compiled regexp's
frobware Mar 10, 2021
3fd480f
Log router write config time
frobware Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/router/template/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ func (r *templateRouter) commitAndReload() error {
reloadStart := time.Now()
err := r.writeConfig()
r.metricWriteConfig.Observe(float64(time.Now().Sub(reloadStart)) / float64(time.Second))
log.V(4).Info("writeConfig", "duration", time.Now().Sub(reloadStart).String())
return err
}(); err != nil {
return err
Expand Down
42 changes: 40 additions & 2 deletions pkg/router/template/template_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"sort"
"strconv"
"strings"
"sync"
"text/template"
"time"

Expand All @@ -31,9 +32,46 @@ func isTrue(s string) bool {
return v
}

// compiledRegexp is the store of already compiled regular
// expressions.
var compiledRegexp sync.Map
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if we needed garbage collection for this map. I believe we do not as every use of firstMatch specifies a pattern that is a hard-coded in the template itself (as opposed to specifying a pattern from an annotation, for example), so the size of the map is bounded by a function of the static set of patterns that are used in haproxy-config.template.


// cachedRegexpCompile will compile pattern using regexp.Compile and
// adds it to the compiledRegexp store if it is not already present.
// It will return an error error if pattern is an invalid regular
// expression. If pattern already exists in the store then no
// compilation is necessary and the existing compiled regexp is
// returned. This provides a huge performance improvement as repeated
// calls to compiling a regular expression is enormous. See:
// https://bugzilla.redhat.com/show_bug.cgi?id=1937972
func cachedRegexpCompile(pattern string) (*regexp.Regexp, error) {
frobware marked this conversation as resolved.
Show resolved Hide resolved
v, ok := compiledRegexp.Load(pattern)
if !ok {
log.V(7).Info("compiling regexp", "pattern", pattern)
re, err := regexp.Compile(pattern)
if err != nil {
return nil, err
}
compiledRegexp.Store(pattern, re)
return re, nil
}
return v.(*regexp.Regexp), nil
}

// matchString reports whether the string s contains any match in
// pattern. Repeated re-compilations of the regular expression
// (pattern) are avoided by utilising the cachedRegexpCompile store.
func matchString(pattern string, s string) (bool, error) {
re, err := cachedRegexpCompile(pattern)
if err != nil {
return false, err
}
return re.MatchString(s), nil
}

func firstMatch(pattern string, values ...string) string {
log.V(7).Info("firstMatch called", "pattern", pattern, "values", values)
if re, err := regexp.Compile(`\A(?:` + pattern + `)\z`); err == nil {
if re, err := cachedRegexpCompile(`\A(?:` + pattern + `)\z`); err == nil {
for _, value := range values {
if re.MatchString(value) {
log.V(7).Info("firstMatch returning", "value", value)
Expand Down Expand Up @@ -80,7 +118,7 @@ func matchValues(s string, allowedValues ...string) bool {

func matchPattern(pattern, s string) bool {
log.V(7).Info("matchPattern called", "pattern", pattern, "s", s)
status, err := regexp.MatchString(`\A(?:`+pattern+`)\z`, s)
status, err := matchString(`\A(?:`+pattern+`)\z`, s)
Copy link
Contributor

Choose a reason for hiding this comment

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

matchPattern isn't used in the template, so this shouldn't affect performance (but it shouldn't hurt anything either).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was explicit as MatchString compiles a regexp too:

// MatchString reports whether the string s
// contains any match of the regular expression pattern.
// More complicated queries need to use Compile and the full Regexp interface.
func MatchString(pattern string, s string) (matched bool, err error) {
	re, err := Compile(pattern)
	if err != nil {
		return false, err
	}
	return re.MatchString(s), nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, this entire function is effectively dead code anyway, unless someone is using a custom template that uses matchPattern. That said, it is possible that someone is using a custom template that uses matchPattern, so we might as well optimize it.

if err == nil {
log.V(7).Info("matchPattern returning", "foundMatch", status)
return status
Expand Down