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

Jump to bottom

Reduce treatment service memory usage #59

Merged
merged 8 commits into from
Jan 19, 2023
Merged

Reduce treatment service memory usage #59

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1741ee
Refactor array iteration when converting interface types into basic d…
deadlycoconuts Jan 19, 2023
4826f53
Remove segments from treatments after they are parsed into sets
deadlycoconuts Jan 19, 2023
b83afc2
Fix unit tests
deadlycoconuts Jan 19, 2023
01e2d4d
Add additional comments and reorder helper method
deadlycoconuts Jan 19, 2023
d753a9d
Refactor boolean map into set map for boolean segment values
deadlycoconuts Jan 19, 2023
d076413
Fix incorrect logic in checkSegmentHasWeakMatch
deadlycoconuts Jan 19, 2023
3c68735
Add todo message for future refactoring
deadlycoconuts Jan 19, 2023
fb285f8
Refactor control statements for readability
deadlycoconuts Jan 19, 2023
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
38 changes: 20 additions & 18 deletions common/utils/typeconverter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ func StringSliceToListSegmenterValue(values *[]string) *_segmenters.ListSegmente
if values == nil {
return nil
}
segmenterValues := []*_segmenters.SegmenterValue{}
for _, val := range *values {
segmenterValues = append(segmenterValues, &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_String_{String_: val},
})
segmenterValues := make([]*_segmenters.SegmenterValue, len(*values))
for i := 0; i < len(*values); i++ {
segmenterValues[i] = &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_String_{String_: (*values)[i]},
}
Comment on lines -16 to +20
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 is a series of minor changes to initialise a fixed-length slice instead of a dynamic length slice to store the segmenter values so as to prevent repeated allocation of memory to copy-paste the slice values when append-ing.

}
return &_segmenters.ListSegmenterValue{Values: segmenterValues}
}
Expand All @@ -26,9 +26,11 @@ func BoolSliceToListSegmenterValue(values *[]bool) *_segmenters.ListSegmenterVal
if values == nil {
return nil
}
segmenterValues := []*_segmenters.SegmenterValue{}
for _, val := range *values {
segmenterValues = append(segmenterValues, &_segmenters.SegmenterValue{Value: &_segmenters.SegmenterValue_Bool{Bool: val}})
segmenterValues := make([]*_segmenters.SegmenterValue, len(*values))
for i := 0; i < len(*values); i++ {
segmenterValues[i] = &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_Bool{Bool: (*values)[i]},
}
}
return &_segmenters.ListSegmenterValue{Values: segmenterValues}
}
Expand All @@ -37,11 +39,11 @@ func Int64ListToListSegmenterValue(values *[]int64) *_segmenters.ListSegmenterVa
if values == nil {
return nil
}
segmenterValues := []*_segmenters.SegmenterValue{}
for _, val := range *values {
segmenterValues = append(segmenterValues, &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_Integer{Integer: val},
})
segmenterValues := make([]*_segmenters.SegmenterValue, len(*values))
for i := 0; i < len(*values); i++ {
segmenterValues[i] = &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_Integer{Integer: (*values)[i]},
}
}
return &_segmenters.ListSegmenterValue{Values: segmenterValues}
}
Expand All @@ -50,11 +52,11 @@ func FloatListToListSegmenterValue(values *[]float64) *_segmenters.ListSegmenter
if values == nil {
return nil
}
segmenterValues := []*_segmenters.SegmenterValue{}
for _, val := range *values {
segmenterValues = append(segmenterValues, &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_Real{Real: val},
})
segmenterValues := make([]*_segmenters.SegmenterValue, len(*values))
for i := 0; i < len(*values); i++ {
segmenterValues[i] = &_segmenters.SegmenterValue{
Value: &_segmenters.SegmenterValue_Real{Real: (*values)[i]},
}
}
return &_segmenters.ListSegmenterValue{Values: segmenterValues}
}
Expand Down
38 changes: 29 additions & 9 deletions treatment-service/models/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ type ExperimentIndex struct {
stringSets map[string]*set.Set
intSets map[string]*set.Set
realSets map[string]*set.Set
boolFlags map[string]bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this new field to the ExperimentIndex struct to store boolean segmenter values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be a set.Set as well. It's possible to have a bool segment as {true, false} if it applies to both values.

Copy link
Contributor Author

@deadlycoconuts deadlycoconuts Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

:O Really? But the original implementation of matchFlagSetSegment seems to suggest that the flag could only be either true or false , esp:

func (i *ExperimentIndex) matchFlagSetSegment(segmentName string, value bool) MatchStrength {
	// ...
	for _, val := range i.Experiment.Segments[segmentName].GetValues() {
		if val.GetBool() == value {
			return MatchStrengthExact
		}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

How? We are iterating over range i.Experiment.Segments[segmentName].GetValues(), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh I see it! Oops my bad, I got carried away with the slightly different implementation of this function as compared to the other match___SetSegment functions and interpreted this differently 😅


StartTime time.Time
EndTime time.Time
Expand Down Expand Up @@ -147,16 +148,12 @@ func (i *ExperimentIndex) MarshalJSON() ([]byte, error) {
}

func (i *ExperimentIndex) matchFlagSetSegment(segmentName string, value bool) MatchStrength {
if _, exists := i.Experiment.Segments[segmentName]; !exists ||
len(i.Experiment.Segments[segmentName].GetValues()) == 0 {
// Optional segmenter
if _, exists := i.boolFlags[segmentName]; !exists {
return MatchStrengthWeak
}

for _, val := range i.Experiment.Segments[segmentName].GetValues() {
if val.GetBool() == value {
return MatchStrengthExact
}
if i.boolFlags[segmentName] == value {
return MatchStrengthExact
}

return MatchStrengthNone
Expand Down Expand Up @@ -204,8 +201,7 @@ func (i *ExperimentIndex) matchRealSetSegment(segmentName string, value float64)
func (i *ExperimentIndex) matchSegment(segmentName string, values []*_segmenters.SegmenterValue) Match {
if len(values) == 0 {
// We can either have an optional match on the experiment or none.
if _, exists := i.Experiment.Segments[segmentName]; !exists ||
len(i.Experiment.Segments[segmentName].GetValues()) == 0 {
if i.checkSegmentHasWeakMatch(segmentName) {
return Match{Strength: MatchStrengthWeak, Value: nil}
}
}
Expand Down Expand Up @@ -238,6 +234,22 @@ func (i *ExperimentIndex) isActive() bool {
return (i.StartTime.Before(time.Now()) || i.StartTime.Equal(time.Now())) && i.EndTime.After(time.Now())
}

func (i *ExperimentIndex) checkSegmentHasWeakMatch(segmentName string) bool {
if set, exists := i.stringSets[segmentName]; !exists || set.Len() == 0 {
krithika369 marked this conversation as resolved.
Show resolved Hide resolved
return true
}
if set, exists := i.intSets[segmentName]; !exists || set.Len() == 0 {
return true
}
if set, exists := i.realSets[segmentName]; !exists || set.Len() == 0 {
return true
}
if _, exists := i.boolFlags[segmentName]; !exists {
return true
}
return false
}

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 new function is basically performing the same purpose as

if _, exists := i.Experiment.Segments[segmentName]; !exists ||
			len(i.Experiment.Segments[segmentName].GetValues()) == 0 {
// ...
}

but has been refactored into a separate helper function and also uses the values within the sets of ExperimentIndex instead of the values stored in ExperimentIndex.Experiment.Segments (which would have been deleted).

func (s *LocalStorage) InsertProjectSettings(projectSettings *pubsub.ProjectSettings) error {
s.Lock()
defer s.Unlock()
Expand Down Expand Up @@ -362,6 +374,7 @@ func NewExperimentIndex(experiment *pubsub.Experiment) *ExperimentIndex {
stringSets := make(map[string]*set.Set)
intSets := make(map[string]*set.Set)
realSets := make(map[string]*set.Set)
boolFlags := make(map[string]bool)

for key, segment := range experiment.Segments {
for _, val := range segment.Values {
Expand All @@ -384,15 +397,22 @@ func NewExperimentIndex(experiment *pubsub.Experiment) *ExperimentIndex {
realSets[key] = set.New()
}
realSets[key].Insert(val.GetReal())
case *_segmenters.SegmenterValue_Bool:
boolFlags[key] = val.GetBool()
}
}
}

// Delete all segments since they have already been converted to the various sets stored in ExperimentIndex,
// and are no longer used by the Treatment Service
experiment.Segments = nil
krithika369 marked this conversation as resolved.
Show resolved Hide resolved

return &ExperimentIndex{
Experiment: experiment,
stringSets: stringSets,
intSets: intSets,
realSets: realSets,
boolFlags: boolFlags,
StartTime: time.Unix(experiment.StartTime.Seconds, 0).UTC(),
EndTime: time.Unix(experiment.EndTime.Seconds, 0).UTC(),
}
Expand Down
5 changes: 4 additions & 1 deletion treatment-service/models/storage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,9 @@ func TestExperimentIndexMatchSegment(t *testing.T) {
realSets: map[string]*set.Set{
"realType": set.New(realSetsVal...),
},
boolFlags: map[string]bool{
"flagType": true,
},
Experiment: &_pubsub.Experiment{
Segments: map[string]*_segmenters.ListSegmenterValue{
"stringType": {
Expand Down Expand Up @@ -672,7 +675,7 @@ func TestExperimentIndexMatchSegment(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := experimentIndex.matchSegment(tt.args.segmentName, tt.args.value)
assert.Equal(t, got, tt.want)
assert.Equal(t, tt.want, got)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flipped this around because assert.Equal was used incorrectly.

})
}
}
Expand Down