Skip to content

Commit

Permalink
Performance improvements for formatPath (#194)
Browse files Browse the repository at this point in the history 
* add performant formatPath

* change GetAttribute to a string and bool return rather than a string pointer

* more descriptive variable name

* creates a new interface for getting attributes w/ (string, bool) and tests for that in formatPath
Adds the original implementation of formatPath for comparison in tests and benchmarks

* type fix

* move type assertion out of the loop

* remove original implementation

* update comments

---------

Co-authored-by: Joel Copi <[email protected]>
Co-authored-by: Mark Vanderwiel <[email protected]>
  • Loading branch information
@jcopi @kramvan1
3 people authored Jan 29, 2025
1 parent a666391 commit 188c2b8
Show file tree
Hide file tree
Showing 6 changed files with 228 additions and 18 deletions.
2 changes: 1 addition & 1 deletion rules/callback_listener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type httpCallbackListener struct {
func (htcbl httpCallbackListener) callbackDone(ruleID string, attributes extendedAttributes) {
attributeMap := make(map[string]string)
for _, k := range attributes.names() {
attributeMap[k] = *attributes.GetAttribute(k)
attributeMap[k], _ = attributes.FindAttribute(k)
}
postObj := callbackEvent{
RuleID: ruleID,
Expand Down
9 changes: 9 additions & 0 deletions rules/dynamic_rule.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,15 @@ func (na *nestingAttributes) GetAttribute(key string) *string {
return na.nested.GetAttribute(key)
}

func (na *nestingAttributes) FindAttribute(key string) (string, bool) {
for _, attribute := range na.attrs {
if attribute.key == key {
return *attribute.value, true
}
}
return na.nested.FindAttribute(key)
}

func (na *nestingAttributes) Format(pattern string) string {
return FormatWithAttributes(pattern, na)
}
Expand Down
71 changes: 55 additions & 16 deletions rules/matcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ type regexKeyMatcher struct {
}

type keyMatch interface {
// GetAttribute usage should be replaced with FindAttribute
GetAttribute(name string) *string
FindAttribute(name string) (string, bool)
Format(pattern string) string
names() []string
}
Expand Down Expand Up @@ -103,6 +105,15 @@ func (m *regexKeyMatch) GetAttribute(name string) *string {
return &result
}

func (m *regexKeyMatch) FindAttribute(name string) (string, bool) {
index, ok := m.fieldMap[name]
if !ok {
return "", false
}
result := m.matchStrings[index]
return result, true
}

func (m *regexKeyMatch) names() []string {
names := make([]string, 0, len(m.fieldMap))
for name := range m.fieldMap {
Expand All @@ -122,28 +133,51 @@ func FormatWithAttributes(pattern string, m Attributes) string {
return result
}

type finderWrapper struct{ Attributes }

func (f finderWrapper) FindAttribute(s string) (string, bool) {
if ptr := f.GetAttribute(s); ptr != nil {
return *ptr, true
}

return "", false
}

func formatPath(pattern string, m Attributes) (string, bool) {
sb := new(strings.Builder)
// If the formatted string can fit into 2.5x the length of the pattern
// (and mapAttributes is the attribute implementation used)
// this will be the only allocation
sb.Grow(2*len(pattern) + (len(pattern) / 2))

var finder AttributeFinder
if f, ok := m.(AttributeFinder); ok {
finder = f
} else {
finder = finderWrapper{m}
}

allFound := true
paths := strings.Split(pattern, "/")
result := strings.Builder{}
for _, path := range paths {
if len(path) == 0 {
continue
}
result.WriteString("/")
if strings.HasPrefix(path, ":") {
attr := m.GetAttribute(path[1:])
if attr == nil {
s := path
attr = &s
var segment string
for found := true; found; {
segment, pattern, found = strings.Cut(pattern, "/")
switch {
case segment == "":
case strings.HasPrefix(segment, ":"):
sb.WriteByte('/')
if attr, ok := finder.FindAttribute(segment[1:]); ok {
sb.WriteString(attr)
} else {
allFound = false
sb.WriteString(segment)
}
result.WriteString(*attr)
} else {
result.WriteString(path)

default:
sb.WriteByte('/')
sb.WriteString(segment)
}
}
return result.String(), allFound
return sb.String(), allFound
}

// Keep the bool return value, because it's tricky to check for null
Expand Down Expand Up @@ -187,6 +221,11 @@ func (ma *mapAttributes) GetAttribute(key string) *string {
return &value
}

func (ma *mapAttributes) FindAttribute(key string) (string, bool) {
value, ok := ma.values[key]
return value, ok
}

func (ma *mapAttributes) Format(path string) string {
return FormatWithAttributes(path, ma)
}
Expand Down
151 changes: 151 additions & 0 deletions rules/matcher_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package rules

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestBasic(t *testing.T) {
Expand Down Expand Up @@ -72,3 +74,152 @@ func TestNoRegex(t *testing.T) {
t.Fail()
}
}

func BenchmarkFormatPath(b *testing.B) {
cases := []struct {
name string
attr Attributes
pattern string
}{
{
name: "03 matches",
attr: NewAttributes(map[string]string{"first": "abcdefghijklomnopqrstuvwxyz", "second": "some_region_name", "third": "acde070d-8c4c-4f0d-9d8a-162843c10333"}),
pattern: "/first/:first/second/:second/third/:third",
},
{
name: "05 matches",
attr: NewAttributes(map[string]string{"a": "abcdefghijklomnopqrstuvwxyz", "b": "abcdefghijklomnopqrstuvwxyz", "c": "acde070d-8c4c-4f0d-9d8a-162843c10333", "d": "acde070d-8c4c-4f0d-9d8a-162843c10333", "e": "acde070d-8c4c-4f0d-9d8a-162843c10333"}),
pattern: "first/:a/second/:b/third/:c/fourth/:d/fifth/:e/sixth",
},
{
name: "10 matches",
attr: NewAttributes(map[string]string{"a": "abcdefghijklomnopqrstuvwxyz", "b": "abcdefghijklomnopqrstuvwxyz", "c": "acde070d-8c4c-4f0d-9d8a-162843c10333", "d": "acde070d-8c4c-4f0d-9d8a-162843c10333", "e": "acde070d-8c4c-4f0d-9d8a-162843c10333"}),
pattern: "first/:a/second/:b/third/:c/fourth/:d/fifth/:e/first/:a/second/:b/third/:c/fourth/:d/fifth/:e/sixth",
},
{
name: "50 matches",
attr: NewAttributes(map[string]string{"param": "acde070d-8c4c-4f0d-9d8a-162843c10333"}),
pattern: strings.Repeat("/:param", 50),
},
{
name: "03 missing",
attr: NewAttributes(map[string]string{"x": "one", "y": "two", "z": "three"}),
pattern: "/first/:first/second/:second/third/:third",
},
{
name: "05 missing",
attr: NewAttributes(map[string]string{"1": "aaaaaaaaaa", "2": "aaaaaaaaaa", "3": "aaaaaaaaaa", "4": "aaaaaaaaaa", "5": "eeeeeeeeee"}),
pattern: "first/:a/second/:b/third/:c/fourth/:d/fifth/:e/sixth",
},
{
name: "10 missing",
attr: NewAttributes(map[string]string{"1": "aaaaaaaaaa", "2": "aaaaaaaaaa", "3": "aaaaaaaaaa", "4": "aaaaaaaaaa", "5": "eeeeeeeeee"}),
pattern: "first/:a/second/:b/third/:c/fourth/:d/fifth/:e/first/:a/second/:b/third/:c/fourth/:d/fifth/:e/sixth",
},
{
name: "50 missing",
attr: NewAttributes(map[string]string{"x": ""}),
pattern: strings.Repeat("/:param", 50),
},
{
name: "all slashes",
attr: NewAttributes(map[string]string{}),
pattern: "////////////////////",
},
{
name: "all patterns",
attr: NewAttributes(map[string]string{}),
pattern: ":/:/:/:/:/:/:/:/:/:/:/:/:/:/:/:/:/:/:/:",
},
}

for _, tc := range cases {
b.Run("curr_"+tc.name, func(b *testing.B) {
for n := 0; n < b.N; n++ {
formatPath(tc.pattern, tc.attr)
}
})
}
}

func TestFormatPath(t *testing.T) {
cases := []struct {
name string
attr Attributes
pattern string
expectstr string
expectbool bool
}{
{
name: "3 matching",
attr: NewAttributes(map[string]string{"first": "one", "second": "two", "third": "three"}),
pattern: "/first/:first/second/:second/third/:third",
expectstr: "/first/one/second/two/third/three",
expectbool: true,
},
{
name: "5 matching",
attr: NewAttributes(map[string]string{"a": "aaaaaaaaaa", "b": "aaaaaaaaaa", "c": "aaaaaaaaaa", "d": "aaaaaaaaaa", "e": "eeeeeeeeee"}),
pattern: "first/:a/second/:b/third/:c/fourth/:d/fifth/:e/sixth",
expectstr: "/first/aaaaaaaaaa/second/aaaaaaaaaa/third/aaaaaaaaaa/fourth/aaaaaaaaaa/fifth/eeeeeeeeee/sixth",
expectbool: true,
},
{
name: "empty segment",
attr: NewAttributes(map[string]string{}),
pattern: "a///b",
expectstr: "/a/b",
expectbool: true,
},
{
name: "empty",
attr: NewAttributes(map[string]string{}),
pattern: "",
expectstr: "",
expectbool: true,
},
{
name: "single slash",
attr: NewAttributes(map[string]string{}),
pattern: "/",
expectstr: "",
expectbool: true,
},
{
name: "empty paths",
attr: NewAttributes(map[string]string{}),
pattern: "///",
expectstr: "",
expectbool: true,
},
{
name: "empty pattern",
attr: NewAttributes(map[string]string{}),
pattern: "/:",
expectstr: "/:",
expectbool: false,
},
{
name: "empty pattern",
attr: NewAttributes(map[string]string{"": "test"}),
pattern: "/:",
expectstr: "/test",
expectbool: true,
},
{
name: "empty value",
attr: NewAttributes(map[string]string{"x": ""}),
pattern: ":x/:x/:x",
expectstr: "///",
expectbool: true,
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
actualstr, actualbool := formatPath(tc.pattern, tc.attr)
require.Equal(t, tc.expectstr, actualstr)
require.Equal(t, tc.expectbool, actualbool)
})
}
}
10 changes: 10 additions & 0 deletions rules/task.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,23 @@ import (
// with dynamic keys. For instance, a dynamic key "/static/:dynamic"
// that is matched against "/static/value1" would contain an yield
// an attribute with the key "dynamic" and the value "value1".
// Attributes implementers should also implement AttributeFinder so that
// the more performant/explicit implementation can be used in internal functions
type Attributes interface {
GetAttribute(string) *string
Format(string) string
}

// AttributeFinder is a more performant replacement for the GetAttribute
// method of Attributes. Internal functions use the FindAttribute method
// if passed an implementation of Attributes that also implements AttributeFinder
type AttributeFinder interface {
FindAttribute(string) (string, bool)
}

type extendedAttributes interface {
Attributes
AttributeFinder
names() []string
}

Expand Down
3 changes: 2 additions & 1 deletion v3enginetest/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ func main() {
// value is set that will prevent further polling even after the polling key TTL has expired.
task.Logger.Info("Callback called")
// This is thread safe, because the map is only being read and not written to.
p := ps[*task.Attr.GetAttribute("id")]
id := *task.Attr.GetAttribute("id")
p := ps[id]
pPollCount := atomic.LoadInt32(&p.pollCount)
// Retrieve a value from etcd.
path := task.Attr.Format(dataPath)
Expand Down

0 comments on commit 188c2b8

Please sign in to comment.