Clear string pointers before returning them to the memory pool. #131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xichen2020
reviewed
Feb 20, 2019
| @@ -5,6 +5,7 @@ import ( | |||
| "testing" | |||
|
|
|||
| "github.com/stretchr/testify/require" | |||
| "github.com/xichen2020/eventdb/x/strings" | |||
index/field/docs_field.go
Outdated
| doubleArrayPool *pool.BucketizedFloat64ArrayPool | ||
| stringArrayPool *pool.BucketizedStringArrayPool | ||
| int64ArrayPool *pool.BucketizedInt64ArrayPool | ||
| stringValuesResetFn strings.ClearArrayFn |
index/field/docs_field.go
Outdated
| @@ -963,3 +965,15 @@ func (o *DocsFieldBuilderOptions) SetInt64ArrayPool(v *pool.BucketizedInt64Array | |||
| func (o *DocsFieldBuilderOptions) Int64ArrayPool() *pool.BucketizedInt64ArrayPool { | |||
| return o.int64ArrayPool | |||
| } | |||
|
|
|||
| // SetStringValuesResetFn sets a value reset function for string values. | |||
| func (o *DocsFieldBuilderOptions) SetStringValuesResetFn(fn strings.ClearArrayFn) *DocsFieldBuilderOptions { | |||
index/field/docs_field.go
Outdated
| } | ||
|
|
||
| // StringValuesResetFn resets string values before returning a string array back to the memory pool. | ||
| func (o *DocsFieldBuilderOptions) StringValuesResetFn() strings.ClearArrayFn { |
storage/mutable_segment.go
Outdated
| @@ -100,7 +101,8 @@ func newMutableSegment( | |||
| SetIntArrayPool(opts.IntArrayPool()). | |||
| SetDoubleArrayPool(opts.DoubleArrayPool()). | |||
| SetStringArrayPool(opts.StringArrayPool()). | |||
| SetInt64ArrayPool(opts.Int64ArrayPool()) | |||
| SetInt64ArrayPool(opts.Int64ArrayPool()). | |||
| SetStringValuesResetFn(strings.ValuesResetFn) | |||
There was a problem hiding this comment.
Also add a comment here to say this is to avoid holding on to documents received after the array is returned to pool.
| @@ -15,7 +18,7 @@ func TestRefCountedPooledStringArray(t *testing.T) { | |||
| pool.Init(func(capacity int) []string { return make([]string, 0, capacity) }) | |||
|
|
|||
| vals := pool.Get(4) | |||
| arr := NewRefCountedPooledStringArray(vals, pool) | |||
| arr := NewRefCountedPooledStringArray(vals, pool, strings.ValuesResetFn) | |||
There was a problem hiding this comment.
Perhaps just leave as nil so you don't need to change the other parts of test?
x/strings/strings.go
Outdated
| package strings | ||
|
|
||
| // ClearArrayFn is the type signature of a string slice clearing fn. | ||
| type ClearArrayFn func(values []string) |
There was a problem hiding this comment.
Just ArrayFn? More generic that way.
x/strings/strings.go
Outdated
| type ClearArrayFn func(values []string) | ||
|
|
||
| // ValuesResetFn clears string pointers inside of a slice of strings. | ||
| func ValuesResetFn(values []string) { |
xichen2020
reviewed
Feb 21, 2019
| @@ -32,24 +33,25 @@ func TestRefCountedPooledStringArray(t *testing.T) { | |||
| require.Equal(t, 4, len(arr.Get())) | |||
| require.Equal(t, int32(2), arr2.cnt.RefCount()) | |||
| require.Equal(t, 4, len(arr2.Get())) | |||
| expected1 := []string{"foo", "bar", "baz", "cat"} | |||
| assertReturnedToStringArrayPool(t, pool, expected1, false) | |||
| assertReturnedToStringArrayPool(t, pool, arr.Get(), false) | |||
There was a problem hiding this comment.
Hm I don't think you need to change the rest of the tests since you already set the reset fn to nil above. I generally prefer to assert against a very explicit value if possible so in this case I prefer to assert against an expected string array instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
String pointers are now cleared before putting string arrays back into the memory pool.