Skip to content

Commit 03ac135

Browse files
weilerNweilerN
authored
[Framework] Improve saferTrie node's value (#71)
### **Motivation** Described in the Jira - https://iguazio.atlassian.net/browse/NUC-486 ### **Root Cause** Memory Layout: ``` []string: 24 bytes (slice header) + 32 bytes (heap allocation for 2 strings) = 56 bytes total CanaryTarget: 32 bytes (stack allocated, no heap allocation) ``` Performance Improvements: - **Creation**: Faster, avoids heap allocation - **Access**: Faster, no pointer dereferencing - **Memory**: ~43% reduction in memory footprint - **Garbage Collection**: Lower GC pressure - **CPU Cache**: Improved locality and access speed ### **Description** This PR introduces a memory-optimized approach for storing values in the `safeTrie`. Instead of using `[]string` to represent function names, this change replaces it with a lightweight struct-based abstraction: `FunctionTarget`. It includes two concrete implementations: - `SingleFunctionTarget` – holds a single function name - `CanaryFunctionTarget` – holds exactly two function names The `FunctionTarget` is propagated to the upper cache layer, and the related interfaces were renamed for clarity and consistency. ### **Affected Areas** Since the only usage of the `FunctionTarget` resides inside the cache, there are no affected areas. ### **Testing** - Added unit tests for SingleFunctionTarget and CanaryFunctionTarget - Updated all relevant cache unit tests to align with the new structure ### **Changes Made** - Introduced the `FunctionTarget` interface with `SingleFunctionTarget` and `CanaryFunctionTarget` implementations - Refactored `safeTrie` and cache layers to support and propagate the new interface - Aligned interface method declarations for readability and consistency with the `FunctionTarget` design ### **Additional Notes** - Expanding the use of the `FunctionTarget` abstraction outside the cache (e.g., surfacing it in the [cache interface](https://github.com/v3io/scaler/blob/development/pkg/ingresscache/types.go#L31)) is left open for further discussion in the code review.
1 parent b73025e commit 03ac135

File tree

5 files changed

+523
-142
lines changed

5 files changed

+523
-142
lines changed

‎pkg/ingresscache/ingresscache.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func (ic *IngressCache) Set(host, path, function string) error {
5050
return errors.Errorf("cache set failed: invalid path tree value: got: %t", urlTree)
5151
}
5252

53-
if err := ingressHostsTree.SetFunctionName(path, function); err != nil {
53+
if err := ingressHostsTree.Set(path, function); err != nil {
5454
return errors.Wrap(err, "failed to set function name in the ingress host tree")
5555
}
5656

@@ -72,7 +72,7 @@ func (ic *IngressCache) Delete(host, path, function string) error {
7272
return errors.Errorf("cache delete failed: invalid path tree value: got: %t", urlTree)
7373
}
7474

75-
if err := ingressHostsTree.DeleteFunctionName(path, function); err != nil {
75+
if err := ingressHostsTree.Delete(path, function); err != nil {
7676
return errors.Wrap(err, "failed to delete function name from the ingress host tree")
7777
}
7878

@@ -97,10 +97,10 @@ func (ic *IngressCache) Get(host, path string) ([]string, error) {
9797
return nil, errors.Errorf("cache get failed: invalid path tree value: got: %t", urlTree)
9898
}
9999

100-
result, err := ingressHostsTree.GetFunctionNames(path)
100+
result, err := ingressHostsTree.Get(path)
101101
if err != nil {
102102
return nil, errors.Wrap(err, "failed to get the function name from the ingress host tree")
103103
}
104104

105-
return result, nil
105+
return result.ToSliceString(), nil
106106
}

‎pkg/ingresscache/ingresscache_test.go‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -123,41 +123,41 @@ func (suite *IngressCacheTestSuite) TestSet() {
123123
args testIngressCacheArgs
124124
expectError bool
125125
errorMessage string
126-
expectedResult map[string]map[string][]string
126+
expectedResult map[string]map[string]FunctionTarget
127127
}{
128128
{
129129
name: "Set new host",
130130
args: testIngressCacheArgs{"example.com", "/test/path", "test-function-name-1"},
131-
expectedResult: map[string]map[string][]string{
132-
"example.com": {"/test/path": {"test-function-name-1"}},
131+
expectedResult: map[string]map[string]FunctionTarget{
132+
"example.com": {"/test/path": SingleTarget("test-function-name-1")},
133133
},
134134
}, {
135135
name: "Set unique functionName that will be added to existing host and path",
136136
args: testIngressCacheArgs{"example.com", "/test/path", "test-function-name-2"},
137137
initialState: []ingressCacheTestInitialState{
138138
{"example.com", "/test/path", "test-function-name-1"},
139139
},
140-
expectedResult: map[string]map[string][]string{
141-
"example.com": {"/test/path": {"test-function-name-1", "test-function-name-2"}},
140+
expectedResult: map[string]map[string]FunctionTarget{
141+
"example.com": {"/test/path": &CanaryTarget{[2]string{"test-function-name-1", "test-function-name-2"}}},
142142
},
143143
}, {
144144
name: "Set existing functionName for existing host and path",
145145
args: testIngressCacheArgs{"example.com", "/test/path", "test-function-name-1"},
146146
initialState: []ingressCacheTestInitialState{
147147
{"example.com", "/test/path", "test-function-name-1"},
148148
},
149-
expectedResult: map[string]map[string][]string{
150-
"example.com": {"/test/path": {"test-function-name-1"}},
149+
expectedResult: map[string]map[string]FunctionTarget{
150+
"example.com": {"/test/path": SingleTarget("test-function-name-1")},
151151
},
152152
}, {
153153
name: "Set another host and path",
154154
args: testIngressCacheArgs{"google.com", "/test/path", "test-function-name-1"},
155155
initialState: []ingressCacheTestInitialState{
156156
{"example.com", "/test/path", "test-function-name-1"},
157157
},
158-
expectedResult: map[string]map[string][]string{
159-
"google.com": {"/test/path": {"test-function-name-1"}},
160-
"example.com": {"/test/path": {"test-function-name-1"}},
158+
expectedResult: map[string]map[string]FunctionTarget{
159+
"google.com": {"/test/path": SingleTarget("test-function-name-1")},
160+
"example.com": {"/test/path": SingleTarget("test-function-name-1")},
161161
},
162162
},
163163
} {
@@ -188,20 +188,20 @@ func (suite *IngressCacheTestSuite) TestDelete() {
188188
expectError bool
189189
errorMessage string
190190
initialState []ingressCacheTestInitialState
191-
expectedResult map[string]map[string][]string
191+
expectedResult map[string]map[string]FunctionTarget
192192
}{
193193
{
194194
name: "Delete when cache is empty",
195195
args: testIngressCacheArgs{"example.com", "/test/path", "test-function-name-1"},
196-
expectedResult: map[string]map[string][]string{},
196+
expectedResult: map[string]map[string]FunctionTarget{},
197197
}, {
198198
name: "Delete not existed host",
199199
args: testIngressCacheArgs{"google.com", "/test/path", "test-function-name-1"},
200200
initialState: []ingressCacheTestInitialState{
201201
{"example.com", "/test/path", "test-function-name-1"},
202202
},
203-
expectedResult: map[string]map[string][]string{
204-
"example.com": {"/test/path": {"test-function-name-1"}},
203+
expectedResult: map[string]map[string]FunctionTarget{
204+
"example.com": {"/test/path": SingleTarget("test-function-name-1")},
205205
},
206206
}, {
207207
name: "Delete last function in host, validate host deletion",
@@ -210,17 +210,17 @@ func (suite *IngressCacheTestSuite) TestDelete() {
210210
{"example.com", "/test/path", "test-function-name-1"},
211211
{"google.com", "/test/path", "test-function-name-1"},
212212
},
213-
expectedResult: map[string]map[string][]string{
214-
"google.com": {"/test/path": {"test-function-name-1"}},
213+
expectedResult: map[string]map[string]FunctionTarget{
214+
"google.com": {"/test/path": SingleTarget("test-function-name-1")},
215215
},
216216
}, {
217217
name: "Delete not existing url and validate host wasn't deleted",
218218
args: testIngressCacheArgs{"example.com", "/not/exists/test/path", "test-function-name-2"},
219219
initialState: []ingressCacheTestInitialState{
220220
{"example.com", "/test/path", "test-function-name-1"},
221221
},
222-
expectedResult: map[string]map[string][]string{
223-
"example.com": {"/test/path": {"test-function-name-1"}},
222+
expectedResult: map[string]map[string]FunctionTarget{
223+
"example.com": {"/test/path": SingleTarget("test-function-name-1")},
224224
},
225225
}, {
226226
name: "Delete not last function in path and validate host wasn't deleted",
@@ -229,8 +229,8 @@ func (suite *IngressCacheTestSuite) TestDelete() {
229229
{"example.com", "/test/path", "test-function-name-1"},
230230
{"example.com", "/test/path", "test-function-name-2"},
231231
},
232-
expectedResult: map[string]map[string][]string{
233-
"example.com": {"/test/path": {"test-function-name-1"}},
232+
expectedResult: map[string]map[string]FunctionTarget{
233+
"example.com": {"/test/path": SingleTarget("test-function-name-1")},
234234
},
235235
},
236236
} {
@@ -270,8 +270,8 @@ func (suite *IngressCacheTestSuite) getTestIngressCache(initialState []ingressCa
270270
}
271271

272272
// flattenIngressCache flattens the IngressCache's syncMap into a map for easier comparison in tests
273-
func (suite *IngressCacheTestSuite) flattenIngressCache(testIngressCache *IngressCache) map[string]map[string][]string {
274-
output := make(map[string]map[string][]string)
273+
func (suite *IngressCacheTestSuite) flattenIngressCache(testIngressCache *IngressCache) map[string]map[string]FunctionTarget {
274+
output := make(map[string]map[string]FunctionTarget)
275275
testIngressCache.syncMap.Range(func(key, value interface{}) bool {
276276
safeTrie, ok := value.(*SafeTrie)
277277
suite.Require().True(ok)

‎pkg/ingresscache/safetrie.go‎

Lines changed: 100 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ such restriction.
2121
package ingresscache
2222

2323
import (
24-
"slices"
2524
"sync"
2625

2726
"github.com/dghubble/trie"
@@ -41,8 +40,8 @@ func NewSafeTrie() *SafeTrie {
4140
}
4241
}
4342

44-
// SetFunctionName sets a function for a given path. If the path does not exist, it creates it
45-
func (st *SafeTrie) SetFunctionName(path string, function string) error {
43+
// Set adds a function for a given path. If the path does not exist, it creates it
44+
func (st *SafeTrie) Set(path string, function string) error {
4645
if path == "" {
4746
return errors.New("path is empty")
4847
}
@@ -57,28 +56,32 @@ func (st *SafeTrie) SetFunctionName(path string, function string) error {
5756
// get the exact path value in order to avoid creating a new path if it already exists
5857
pathValue := st.pathTrie.Get(path)
5958
if pathValue == nil {
60-
st.pathTrie.Put(path, []string{function})
59+
st.pathTrie.Put(path, SingleTarget(function))
6160
return nil
6261
}
6362

64-
pathFunctionNames, ok := pathValue.([]string)
63+
pathFunctionNames, ok := pathValue.(FunctionTarget)
6564
if !ok {
66-
return errors.Errorf("path value should be []string, got %T", pathValue)
65+
return errors.Errorf("path value should be FunctionTarget, got %T", pathValue)
6766
}
6867

69-
if slices.Contains(pathFunctionNames, function) {
70-
// If the function already exists at this path, skip adding it to prevent duplicates
68+
if pathFunctionNames.Contains(function) {
69+
// Although Add() checks if the function exists and returns the same value, it still performs a trie walk that ends with no changes when values are identical.
70+
// This validation avoids that unnecessary walk
7171
return nil
7272
}
7373

74-
pathFunctionNames = append(pathFunctionNames, function)
75-
st.pathTrie.Put(path, pathFunctionNames)
74+
functionNames, err := pathFunctionNames.Add(function)
75+
if err != nil {
76+
return errors.Wrapf(err, "failed to set function name to path. path: %s, function: %s", path, function)
77+
}
7678

79+
st.pathTrie.Put(path, functionNames)
7780
return nil
7881
}
7982

80-
// DeleteFunctionName removes a function from a path and also deletes the path if the function is the only one associated with that path
81-
func (st *SafeTrie) DeleteFunctionName(path string, function string) error {
83+
// Delete removes a function from a path and cleans up the longest suffix of the path only used by that function
84+
func (st *SafeTrie) Delete(path string, function string) error {
8285
st.rwMutex.Lock()
8386
defer st.rwMutex.Unlock()
8487

@@ -88,27 +91,30 @@ func (st *SafeTrie) DeleteFunctionName(path string, function string) error {
8891
return nil
8992
}
9093

91-
pathFunctionNames, ok := pathValue.([]string)
94+
pathFunctionNames, ok := pathValue.(FunctionTarget)
9295
if !ok {
93-
return errors.Errorf("path value should be []string, got %T", pathValue)
96+
return errors.Errorf("path value should be FunctionTarget, got %T", pathValue)
9497
}
9598

96-
// If the function to delete matches the current function name and it's the only value, delete the path
97-
if len(pathFunctionNames) == 1 {
98-
if pathFunctionNames[0] == function {
99+
// If the function to delete matches the current function name, and it's the only value, delete the path
100+
if pathFunctionNames.IsSingle() {
101+
if pathFunctionNames.Contains(function) {
99102
st.pathTrie.Delete(path)
100103
}
101104
return nil
102105
}
103106

104-
// TODO - will be removed once moving into efficient pathFunctionNames implementation (i.e. not using slices)
105-
pathFunctionNames = excludeElemFromSlice(pathFunctionNames, function)
106-
st.pathTrie.Put(path, pathFunctionNames)
107+
// update the path with the new function names after deletion
108+
functionNames, err := pathFunctionNames.Delete(function)
109+
if err != nil {
110+
return errors.Wrapf(err, "failed to remove function name from path. function: %s, path: %s", function, path)
111+
}
112+
st.pathTrie.Put(path, functionNames)
107113
return nil
108114
}
109115

110-
// GetFunctionNames retrieve the closest prefix matching the path and returns the associated functions
111-
func (st *SafeTrie) GetFunctionNames(path string) ([]string, error) {
116+
// Get retrieve the closest prefix matching the path and returns the associated functions
117+
func (st *SafeTrie) Get(path string) (FunctionTarget, error) {
112118
var walkPathResult interface{}
113119
if path == "" {
114120
return nil, errors.New("path is empty")
@@ -127,9 +133,9 @@ func (st *SafeTrie) GetFunctionNames(path string) ([]string, error) {
127133
return nil, errors.Errorf("no value found for path: %s", path)
128134
}
129135

130-
functionNames, ok := walkPathResult.([]string)
136+
functionNames, ok := walkPathResult.(FunctionTarget)
131137
if !ok {
132-
return nil, errors.Errorf("walkPathResult value should be []string, got %v", walkPathResult)
138+
return nil, errors.Errorf("walkPathResult value should be FunctionTarget, got %v", walkPathResult)
133139
}
134140

135141
return functionNames, nil
@@ -146,25 +152,75 @@ func (st *SafeTrie) IsEmpty() bool {
146152
return walkResult == nil
147153
}
148154

149-
// TODO - will be removed once moving into efficient pathFunctionNames implementation (i.e. not using slices)
150-
func excludeElemFromSlice(slice []string, elem string) []string {
151-
// Assuming len(slice) <= 2 based on the ingress validations of: https://github.com/nuclio/nuclio/pkg/platform/kube/platform.go
152-
switch len(slice) {
153-
case 1:
154-
if slice[0] == elem {
155-
return []string{}
156-
}
157-
return slice
158-
case 2:
159-
if slice[0] == elem {
160-
return []string{slice[1]}
161-
}
162-
if slice[1] == elem {
163-
return []string{slice[0]}
164-
}
165-
// elem not found, return original slice
166-
return slice
167-
default:
168-
return slice
155+
// ----- implementations for FunctionTarget interface -----
156+
157+
type SingleTarget string
158+
159+
func (s SingleTarget) Contains(functionName string) bool {
160+
return string(s) == functionName
161+
}
162+
163+
func (s SingleTarget) Delete(functionName string) (FunctionTarget, error) {
164+
if !s.Contains(functionName) {
165+
// if the function name is not found, return the original SingleTarget
166+
return s, nil
167+
}
168+
169+
// this should never be called for SingleTarget
170+
return nil, errors.New("cannot remove function name from SingleTarget, it only contains one function name")
171+
}
172+
173+
func (s SingleTarget) Add(functionName string) (FunctionTarget, error) {
174+
if s.Contains(functionName) {
175+
return s, nil
176+
}
177+
178+
return &CanaryTarget{functionNames: [2]string{string(s), functionName}}, nil
179+
}
180+
181+
func (s SingleTarget) ToSliceString() []string {
182+
return []string{string(s)}
183+
}
184+
185+
func (s SingleTarget) IsSingle() bool {
186+
return true
187+
}
188+
189+
type CanaryTarget struct {
190+
functionNames [2]string
191+
}
192+
193+
func (c *CanaryTarget) Contains(functionName string) bool {
194+
return c.functionNames[0] == functionName || c.functionNames[1] == functionName
195+
}
196+
197+
func (c *CanaryTarget) Delete(functionName string) (FunctionTarget, error) {
198+
if c.functionNames[0] == functionName {
199+
return SingleTarget(c.functionNames[1]), nil
200+
}
201+
202+
if c.functionNames[1] == functionName {
203+
return SingleTarget(c.functionNames[0]), nil
169204
}
205+
206+
// if reached here, it means CanaryTarget does not contain the function name
207+
return c, nil
208+
}
209+
210+
func (c *CanaryTarget) Add(functionName string) (FunctionTarget, error) {
211+
if c.Contains(functionName) {
212+
// If the function already exists, return the original CanaryTarget
213+
return c, nil
214+
}
215+
216+
// This should never be called for CanaryTarget since it should always contain exactly two function names
217+
return c, errors.New("cannot add function name to CanaryTarget, it already contains two function names")
218+
}
219+
220+
func (c *CanaryTarget) ToSliceString() []string {
221+
return []string{c.functionNames[0], c.functionNames[1]}
222+
}
223+
224+
func (c *CanaryTarget) IsSingle() bool {
225+
return false
170226
}

0 commit comments

Comments
 (0)