implement cache abstraction and unit test #38

p0mvn · 2022-03-27T22:45:41Z

Context

See here: Implement configurable fast node cache size osmosis#1163
To sum up:
- better abstraction for cache to eventually enable the limit by bytes as opposed to the number of nodes
- unit tested cache
- avoid caching nodes with the same key (the old one is now replaced)
- cache adapted from: https://github.com/golang/groupcache/blob/master/lru/lru.go

Benchstat

name                                                                 old time/op    new time/op    delta
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-fast-4    4.22µs ± 9%    4.15µs ± 6%     ~     (p=1.000 n=5+5)
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-slow-4    15.3µs ±11%    16.6µs ± 6%     ~     (p=0.056 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-fast-4                     566ns ± 8%     613ns ±11%     ~     (p=0.222 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-slow-4                    21.2µs ± 5%    21.0µs ± 4%     ~     (p=0.690 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-fast-4                     78.0ms ± 6%    81.1ms ±11%     ~     (p=0.548 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-slow-4                      1.79s ±13%     1.90s ±11%     ~     (p=0.222 n=5+5)
Medium/goleveldb-100000-100-16-40/update-4                              233µs ±15%     203µs ± 8%  -12.78%  (p=0.008 n=5+5)
Medium/goleveldb-100000-100-16-40/block-4                              29.0ms ± 4%    25.8ms ±10%  -11.04%  (p=0.016 n=5+5)

name                                                                 old alloc/op   new alloc/op   delta
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-fast-4      814B ± 0%      814B ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-slow-4    1.41kB ± 1%    1.41kB ± 0%     ~     (p=1.000 n=5+5)
Medium/goleveldb-100000-100-16-40/query-hits-fast-4                     0.00B          0.00B          ~     (all equal)
Medium/goleveldb-100000-100-16-40/query-hits-slow-4                    1.99kB ± 1%    2.00kB ± 1%     ~     (p=0.460 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-fast-4                     29.3MB ± 0%    29.3MB ± 0%     ~     (p=0.381 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-slow-4                      276MB ± 0%     276MB ± 0%   +0.02%  (p=0.029 n=4+4)
Medium/goleveldb-100000-100-16-40/update-4                             47.4kB ± 2%    46.5kB ± 4%     ~     (p=0.151 n=5+5)
Medium/goleveldb-100000-100-16-40/block-4                              5.65MB ± 2%    5.41MB ± 2%   -4.27%  (p=0.008 n=5+5)

name                                                                 old allocs/op  new allocs/op  delta
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-fast-4      16.0 ± 0%      16.0 ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/query-no-in-tree-guarantee-slow-4      24.0 ± 0%      24.0 ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/query-hits-fast-4                      0.00           0.00          ~     (all equal)
Medium/goleveldb-100000-100-16-40/query-hits-slow-4                      34.0 ± 0%      34.0 ± 0%     ~     (all equal)
Medium/goleveldb-100000-100-16-40/iteration-fast-4                       523k ± 0%      523k ± 0%     ~     (p=0.484 n=5+5)
Medium/goleveldb-100000-100-16-40/iteration-slow-4                      4.71M ± 0%     4.71M ± 0%   +0.01%  (p=0.029 n=4+4)
Medium/goleveldb-100000-100-16-40/update-4                                502 ± 9%       482 ± 6%     ~     (p=0.421 n=5+5)
Medium/goleveldb-100000-100-16-40/block-4                               62.1k ± 3%     59.6k ± 2%   -4.05%  (p=0.016 n=5+5)

alexanderbez

LGTM

alexanderbez · 2022-03-28T14:39:45Z

cache/cache.go

+type Node interface {
+	GetKey() []byte
+}
+
+type Cache interface {


nit: add godocs to what these interfaces are :)

alexanderbez · 2022-03-28T14:40:24Z

cache/cache.go

+}
+
+// lruCache is an LRU cache implementeation.
+type lruCache struct {


I think formatting might be off here?

alexanderbez · 2022-03-28T14:41:43Z

cache/cache.go

+	}
+}
+
+func (nc *lruCache) Add(node Node) Node {


nit: I'd call the method receiver c instead of nc

alexanderbez · 2022-03-28T14:43:11Z

cache/cache_test.go

+	}
+}
+
+func BenchmarkAdd(b *testing.B) {


I've typically seen benchmarks separated out into a dedicated *_bench_test.go file so they don't clobber with the unit tests. WDYT?

Good idea. Although this is not a convention in this repo, I think it's good to slowly start doing that. Moved cache benchmarks into a separate file cache_bench_test.go

alexanderbez · 2022-03-28T14:44:03Z

nodedb.go

+		opts:           *opts,
+		latestVersion:  0, // initially invalid
+		nodeCache:      cache.New(cacheSize),
+		fastNodeCache:  cache.New(100000),


should we put this into a const?

p0mvn · 2022-03-28T20:32:51Z

Hey @alexanderbez . Thanks for reviewing. Addressed all your comments. Just checking in if you are okay with me merging?

alexanderbez · 2022-03-28T23:01:40Z

@p0mvn LGTM! :)

I think there are some linting and test failures in CI, is that expected?

p0mvn · 2022-03-29T00:13:40Z

Yes, that's expected for now. I'm planning on fixing the linter as soon as I have some extra time

implementing cache abstraction, unit test Add

0f682da

p0mvn mentioned this pull request Mar 27, 2022

Implement configurable fast node cache size osmosis-labs/osmosis#1163

Closed

8 tasks

p0mvn added 4 commits March 28, 2022 01:50

create benchmark for Add(), start working on benchmark for Remove()

db171c0

implement no duplicates in cache and unit test

a46d063

switch fast node cache to the new abstraction

46fd9e9

switch regular cache to the new abstraction

da5362f

p0mvn changed the title ~~implementing cache abstraction and unit test~~ implement cache abstraction and unit test Mar 28, 2022

p0mvn linked an issue Mar 28, 2022 that may be closed by this pull request

Refactor fastNodeCache and nodeCache to be separate modules with synchrony #24

Closed

9 tasks

p0mvn marked this pull request as ready for review March 28, 2022 13:25

p0mvn requested review from alexanderbez, ValarDragon and czarcas7ic March 28, 2022 13:27

alexanderbez approved these changes Mar 28, 2022

View reviewed changes

p0mvn and others added 4 commits March 28, 2022 14:54

fmt and add interface godoc

cde3137

rename receiver to c from nc

a01fa14

const fastNodeCacheLimit

60ceb6f

move benchmarks to a separate file

8048be0

p0mvn merged commit 8657fee into dev/iavl_data_locality Mar 29, 2022

p0mvn mentioned this pull request Jun 14, 2022

refactor: implement cache abstraction and unit test cosmos/iavl#506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement cache abstraction and unit test #38

implement cache abstraction and unit test #38

p0mvn commented Mar 27, 2022 •

edited

Loading

alexanderbez left a comment

alexanderbez Mar 28, 2022

p0mvn Mar 28, 2022

alexanderbez Mar 28, 2022

p0mvn Mar 28, 2022

alexanderbez Mar 28, 2022

p0mvn Mar 28, 2022

alexanderbez Mar 28, 2022

p0mvn Mar 28, 2022

alexanderbez Mar 28, 2022

p0mvn commented Mar 28, 2022

alexanderbez commented Mar 28, 2022 •

edited

Loading

p0mvn commented Mar 29, 2022

implement cache abstraction and unit test #38

implement cache abstraction and unit test #38

Conversation

p0mvn commented Mar 27, 2022 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Mar 28, 2022

alexanderbez commented Mar 28, 2022 • edited Loading

p0mvn commented Mar 29, 2022

p0mvn commented Mar 27, 2022 •

edited

Loading

alexanderbez commented Mar 28, 2022 •

edited

Loading