Bug 1937972: router/template: Cache compiled regular expressions #268
Conversation
openshift-ci-robot
added
the
approved
|
Looks good to me! But waiting for a second reviewer. |
| @@ -80,7 +105,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) | |||
There was a problem hiding this comment.
matchPattern isn't used in the template, so this shouldn't affect performance (but it shouldn't hurt anything either).
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
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.
| @@ -31,9 +32,33 @@ func isTrue(s string) bool { | |||
| return v | |||
| } | |||
|
|
|||
| var compiledRegexp sync.Map | |||
There was a problem hiding this comment.
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.
openshift-ci-robot
added
the
do-not-merge/hold
|
/retitle Bug 1937972: router/template: Cache compiled regular expressions |
openshift-ci-robot
changed the title
openshift-ci-robot
added
the
bugzilla/severity-high
|
@frobware: This pull request references Bugzilla bug 1937972, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
frobware
force-pushed
the
cache-compiled-regexps
branch
from
dafee34 to
e9d22d6
Compare
frobware
force-pushed
the
cache-compiled-regexps
branch
from
e9d22d6 to
b96fe3b
Compare
| } | ||
|
|
||
| func matchString(pattern string, s string) (bool, error) { | ||
| status, err := regexp.MatchString(`\A(?:`+pattern+`)\z`, s) |
There was a problem hiding this comment.
What's this line for? Copy and paste error?
There was a problem hiding this comment.
Errr. yes. Removing. :)
frobware
force-pushed
the
cache-compiled-regexps
branch
from
b96fe3b to
3fd480f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware, knobunc, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@frobware: All pull requests linked via external trackers have merged: Bugzilla bug 1937972 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.7 |
|
@frobware: new pull request created: #269 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.6 |
|
@frobware: new pull request created: #270 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.5 |
|
@frobware: new pull request created: #271 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This is an optimisation that caches compiled regular expressions.
When the template is written calls to
matchStringandfirstMatchwould recompile the regular expression on every call. When you are
processing ~13,000 routes the time to compile on each invocation becomes
significant. This change brings a reduction of 60% when processing and
writing
haproxy.config.And if we cache the REs:
Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1929821