forked from cosmos/iavl
-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
add byte limit implementation for cache #40
Open
p0mvn
wants to merge
24
commits into
dev/iavl_data_locality
Choose a base branch
from
roman/fast-cache-bytes
base: dev/iavl_data_locality
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
1b00d89
move lruCacheNodeLimit implementation into a separate file
p0mvn 86f8f5e
rename cache_test to cache_node_limit_test.go
p0mvn a9e8eaa
implment and test GetFullSize for fast nodes
p0mvn 2037ed2
refactor cache to minimize duplication between various implementations
p0mvn c9b0397
implement bytes limit cache, add simple unit tests for add
p0mvn bd8273b
rename decorators
p0mvn 557613c
add function to test current bytes
p0mvn 16cc236
finish bytes limit add tests and fmt
p0mvn 7645c4c
do not return updated elements
p0mvn 652d9f0
add godoc
p0mvn 31097db
switch fast node cache to 100MB
p0mvn c3787d0
add cache tests to makefile and CI
p0mvn d53703f
fix 32 bit test in fast_node_test.go
p0mvn 80febdc
restore Makefile and ci.yml - should still run cache tests
p0mvn 734412b
unit test bytes limit remove
p0mvn 0b39cff
reuse code between tests
p0mvn 0363e41
fmt
p0mvn 9b3ccb4
refactor add tests to reuse code
p0mvn edec9c3
add benchmarks
p0mvn 6e5b9cd
fmt
p0mvn fec7c9a
Update fast_node_test.go
p0mvn b9ee524
add godocs for cache.Node implementations and fmt
p0mvn 076dc56
rename fastNodeCacheLimit to fastNodeCacheLimitBytes
p0mvn b59ba72
add comment about curBytesEstimate
p0mvn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
package cache | ||
|
||
import ( | ||
"container/list" | ||
"errors" | ||
|
||
"github.com/cosmos/iavl/common" | ||
) | ||
|
||
type lruCacheWithBytesLimit struct { | ||
lruCache | ||
bytesLimit int | ||
// This is called an estimate because we calculcate the number of bytes used based on the | ||
// knowledge of how the underlying structs, slices and strings are represented in Go. | ||
// Since Go is a garbage collected language, there is no good way to retrieve the exact number | ||
// of bytes used by both the stack and the heap at any particular moment. As a result, this | ||
// is named an estimate to reflect that it is not a precise number. | ||
curBytesEstimate int | ||
} | ||
|
||
var _ Cache = (*lruCacheWithBytesLimit)(nil) | ||
|
||
func NewWithBytesLimit(bytesLimit int) Cache { | ||
return &lruCacheWithBytesLimit{ | ||
lruCache: lruCache{ | ||
dict: make(map[string]*list.Element), | ||
ll: list.New(), | ||
}, | ||
bytesLimit: bytesLimit, | ||
} | ||
} | ||
|
||
func (c *lruCacheWithBytesLimit) GetType() Type { | ||
return LRU_bytes_limit | ||
} | ||
|
||
func (c *lruCacheWithBytesLimit) isOverLimit() bool { | ||
return c.curBytesEstimate > c.bytesLimit | ||
} | ||
|
||
func (c *lruCacheWithBytesLimit) add(node Node) { | ||
c.curBytesEstimate += (node.GetFullSize() + getCacheElemMetadataSize()) | ||
c.lruCache.add(node) | ||
} | ||
|
||
func (c *lruCacheWithBytesLimit) remove(e *list.Element) Node { | ||
removed := c.lruCache.remove(e) | ||
c.curBytesEstimate -= (removed.GetFullSize() + getCacheElemMetadataSize()) | ||
return removed | ||
} | ||
|
||
// getCacheElemMetadataSize returns how much space the structures | ||
// that hold a cache element utilize in memory. | ||
// With the current design, a list.Element is created that consists of 4 pointers. | ||
// In addition, a pointer to the element is stored in the Go map and has string as a key | ||
func getCacheElemMetadataSize() int { | ||
return common.GetStringSizeBytes() + // cache dict key | ||
common.UintSizeBytes + // pointer to the element in dict | ||
common.Uint64Size*4 // 4 pointers within list.Element | ||
} | ||
|
||
// getCacheCurrentBytes returns the current bytes | ||
// estimate of the cache if the cache is lruCacheWithBytesLimit. | ||
// If not, returns 0 and error. | ||
func getCacheCurrentBytes(c Cache) (int, error) { | ||
withBytesLimit, ok := c.(*lruCacheWithBytesLimit) | ||
if ok { | ||
return withBytesLimit.curBytesEstimate, nil | ||
} | ||
return 0, errors.New("cannot get bytes limit, not lruCacheWithBytesLimit") | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,248 @@ | ||
package cache_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/cosmos/iavl/cache" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func Test_BytesLimit_Add(t *testing.T) { | ||
testcases := map[string]func() testcase{ | ||
"add 1 node with size of exactly limit - added": func() testcase { | ||
const nodeIdx = 0 | ||
|
||
return testcase{ | ||
cacheLimit: testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: 0, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
}, | ||
}, | ||
expectedNodeIndexes: []int{nodeIdx}, | ||
} | ||
}, | ||
"add 2 nodes with latter exceeding limit - added, old removed": func() testcase { | ||
const nodeIdx = 0 | ||
|
||
return testcase{ | ||
cacheLimit: testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx + 1, | ||
expectedResult: nodeIdx, | ||
expectedBytesLimit: testNodes[nodeIdx+1].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
}, | ||
}, | ||
expectedNodeIndexes: []int{nodeIdx + 1}, | ||
} | ||
}, | ||
"add 2 nodes under limit": func() testcase { | ||
const nodeIdx = 0 | ||
|
||
return testcase{ | ||
cacheLimit: testNodes[nodeIdx].GetFullSize() + testNodes[nodeIdx+3].GetFullSize() + 2*cache.GetCacheElemMetadataSize(), | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx + 3, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + testNodes[nodeIdx+3].GetFullSize() + 2*cache.GetCacheElemMetadataSize(), | ||
}, | ||
}, | ||
expectedNodeIndexes: []int{nodeIdx, nodeIdx + 3}, | ||
} | ||
}, | ||
"add 3 nodes and 4th requiring the removal of first three due to being too large": func() testcase { | ||
const nodeIdx = 0 | ||
|
||
return testcase{ | ||
cacheLimit: testNodes[nodeIdx].GetFullSize() + | ||
testNodes[nodeIdx+1].GetFullSize() + | ||
testNodes[nodeIdx+2].GetFullSize() + | ||
3*cache.GetCacheElemMetadataSize(), | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize(), | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx + 1, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + testNodes[nodeIdx+1].GetFullSize() + 2*cache.GetCacheElemMetadataSize(), | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx + 2, | ||
expectedResult: noneRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx].GetFullSize() + testNodes[nodeIdx+1].GetFullSize() + testNodes[nodeIdx+2].GetFullSize() + 3*cache.GetCacheElemMetadataSize(), | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx + 3, | ||
expectedResult: allButLastRemoved, | ||
expectedBytesLimit: testNodes[nodeIdx+2].GetFullSize() + testNodes[nodeIdx+3].GetFullSize() + 2*cache.GetCacheElemMetadataSize(), | ||
}, | ||
}, | ||
expectedNodeIndexes: []int{nodeIdx + 2, nodeIdx + 3}, | ||
} | ||
}, | ||
} | ||
|
||
for name, getTestcaseFn := range testcases { | ||
t.Run(name, func(t *testing.T) { | ||
tc := getTestcaseFn() | ||
bytesLimitCache := cache.NewWithBytesLimit(tc.cacheLimit) | ||
testAdd(t, bytesLimitCache, tc) | ||
}) | ||
} | ||
} | ||
|
||
func Test_BytesLimitCache_Remove(t *testing.T) { | ||
testcases := map[string]func() testcase{ | ||
"remove non-existent key, cache limit 0 - nil returned": func() testcase { | ||
const ( | ||
nodeIdx = 0 | ||
cacheLimit = 0 | ||
) | ||
|
||
return testcase{ | ||
cacheLimit: cacheLimit, | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: noneRemoved, | ||
}, | ||
}, | ||
} | ||
}, | ||
"remove non-existent key - nil returned": func() testcase { | ||
const ( | ||
nodeIdx = 0 | ||
) | ||
|
||
var ( | ||
cacheLimit = testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize() | ||
) | ||
|
||
return testcase{ | ||
setup: func(c cache.Cache) { | ||
require.Empty(t, cache.MockAdd(c, testNodes[nodeIdx+1])) | ||
require.Equal(t, 1, c.Len()) | ||
}, | ||
cacheLimit: cacheLimit, | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: noneRemoved, | ||
}, | ||
}, | ||
expectedNodeIndexes: []int{1}, | ||
} | ||
}, | ||
"remove existent key - removed": func() testcase { | ||
const ( | ||
nodeIdx = 0 | ||
) | ||
|
||
var ( | ||
cacheLimit = testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize() | ||
) | ||
|
||
return testcase{ | ||
setup: func(c cache.Cache) { | ||
require.Empty(t, cache.MockAdd(c, testNodes[nodeIdx])) | ||
require.Equal(t, 1, c.Len()) | ||
}, | ||
cacheLimit: cacheLimit, | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: nodeIdx, | ||
}, | ||
}, | ||
} | ||
}, | ||
"remove twice, cache limit 1 - removed first time, then nil": func() testcase { | ||
const ( | ||
nodeIdx = 0 | ||
) | ||
|
||
var ( | ||
cacheLimit = testNodes[nodeIdx].GetFullSize() + cache.GetCacheElemMetadataSize() | ||
) | ||
|
||
return testcase{ | ||
setup: func(c cache.Cache) { | ||
require.Empty(t, cache.MockAdd(c, testNodes[nodeIdx])) | ||
require.Equal(t, 1, c.Len()) | ||
}, | ||
cacheLimit: cacheLimit, | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: nodeIdx, | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: noneRemoved, | ||
}, | ||
}, | ||
} | ||
}, | ||
"remove all, cache limit 3": func() testcase { | ||
const ( | ||
nodeIdx = 0 | ||
) | ||
|
||
var ( | ||
cacheLimit = testNodes[nodeIdx].GetFullSize() + | ||
testNodes[nodeIdx+1].GetFullSize() + | ||
testNodes[nodeIdx+2].GetFullSize() + | ||
3*cache.GetCacheElemMetadataSize() | ||
) | ||
|
||
return testcase{ | ||
setup: func(c cache.Cache) { | ||
require.Empty(t, cache.MockAdd(c, testNodes[nodeIdx])) | ||
require.Empty(t, cache.MockAdd(c, testNodes[nodeIdx+1])) | ||
require.Empty(t, cache.MockAdd(c, testNodes[nodeIdx+2])) | ||
require.Equal(t, 3, c.Len()) | ||
}, | ||
cacheLimit: cacheLimit, | ||
cacheOps: []cacheOp{ | ||
{ | ||
testNodexIdx: nodeIdx + 2, | ||
expectedResult: nodeIdx + 2, | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx, | ||
expectedResult: nodeIdx, | ||
}, | ||
{ | ||
testNodexIdx: nodeIdx + 1, | ||
expectedResult: nodeIdx + 1, | ||
}, | ||
}, | ||
} | ||
}, | ||
} | ||
|
||
for name, getTestcaseFn := range testcases { | ||
t.Run(name, func(t *testing.T) { | ||
tc := getTestcaseFn() | ||
bytesLimitCache := cache.NewWithNodeLimit(tc.cacheLimit) | ||
testRemove(t, bytesLimitCache, tc) | ||
}) | ||
} | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it called estimate? Should we add a comment that it may potentially slightly undercount, but we deem it fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is called estimate because I did not find a straightforward way to test actual allocations.
I tried grabbing memory from runtime to test but it was slightly off. I think it's because it doesn't produce the memory needed for allocating "slice metadata" - length, capacity and pointer to data.
This estimate is based on the knowledge of how slices and strings are represented in memory in Go. That's why I named it estimate.
I'll add a comment about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh gotcha. Yeah being off by a small constant for go memory layout details (which aren't guaranteed to be preserved across versions) is totally fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is added