Reduce treatment service memory usage #59
Conversation
| 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]}, | ||
| } |
There was a problem hiding this comment.
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.
treatment-service/models/storage.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Added this new field to the ExperimentIndex struct to store boolean segmenter values.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
: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
}
}There was a problem hiding this comment.
How? We are iterating over range i.Experiment.Segments[segmentName].GetValues(), no?
There was a problem hiding this comment.
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 😅
treatment-service/models/storage.go
Outdated
| func (i *ExperimentIndex) checkSegmentHasWeakMatch(segmentName string) bool { | ||
| if set, exists := i.stringSets[segmentName]; !exists || set.Len() == 0 { | ||
| 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 | ||
| } | ||
|
|
There was a problem hiding this comment.
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).
| @@ -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) | |||
There was a problem hiding this comment.
Flipped this around because assert.Equal was used incorrectly.
treatment-service/models/storage.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
|
Thanks a lot for the quick review! I'll be merging this now! 🚀 |
What this PR does / why we need it:
This PR introduces some minor refactoring that reduces the memory usage of the Treatment Service by removing the redundancy in storing segmenter values twice in the
ExperimentIndexstruct:ExperimentIndex.stringSets,ExperimentIndex.intSets,ExperimentIndex.realSetsExperimentIndex.Experiment.SegmentsGiven that the Treatment Service only uses the sets of an
ExperimentIndexto determine a matching experiment, there is no need to store the original copy withinExperimentIndex.Experiment.Segments. While this duplication might seem minor and negligible, we have observed instances whereby experiments with a huge number of segmenter values cause a huge increase in memory usage by the Treatment Service due to the duplication of the data stored in memory.This PR simply removes the values in
ExperimentIndex.Experiment.Segmentsonce they have been used to initialise the various sets, to reduce the overall memory footprint of the Treatment Service. Additional changes to existing functions are also made to accommodate the removal of that field.