WIP: [x/auth] Add lru cache for account #2678

yutianwu · 2018-11-04T10:53:15Z

Add a lru cache for Account because getting and setting accounts happened frequently(CheckTx and DeliverTx) and decoding is time-consuming.

codecov · 2018-11-04T11:01:06Z

Codecov Report

Merging #2678 into develop will increase coverage by 6.65%.
The diff coverage is 91.66%.

@@             Coverage Diff             @@
##           develop    #2678      +/-   ##
===========================================
+ Coverage    52.17%   58.83%   +6.65%     
===========================================
  Files          136      152      +16     
  Lines         9619     9481     -138     
===========================================
+ Hits          5019     5578     +559     
+ Misses        4263     3532     -731     
- Partials       337      371      +34

alexanderbez

Thanks @yutianwu! I left some remarks.

alexanderbez · 2018-11-04T13:55:27Z

Gopkg.toml

@@ -88,6 +88,10 @@
  name = "github.com/mitchellh/go-homedir"
  version = "1.0.0"

+[[constraint]]


All for using this lib, although we should remove our own LRUs now.

I'm not sure I agree, its kind of a simple thing to introduce a new dependency for. We already have a self-created LRU in tendermint.

Where is our generalized LRU cache? This is the most commonly used LRU lib I've seen.

@ValarDragon is probably referring to https://github.com/tendermint/tendermint/blob/c086d0a34102bd42873d20445673ea1d18a539cd/mempool/mempool.go#L595, but it's not generalized (not to the extent of hashicorp's LRU).

alexanderbez · 2018-11-04T13:56:25Z

x/auth/account.go

@@ -121,6 +123,35 @@ func (acc *BaseAccount) SetSequence(seq int64) error {
 	return nil
 }

+// Implements sdk.Account.


Suggested change

// Implements sdk.Account.

// Clone creates a deep copy of a BaseAccount. It implements the sdk.Account interface.

alexanderbez · 2018-11-04T13:57:13Z

x/auth/account.go

+	// it should be fine if not deep copy them. if both of
+	// the two interfaces can provide a Clone() method would be terrific.
+	clonedAcc := &BaseAccount{
+		PubKey:        acc.PubKey,


We don't need to set PubKey or Address here. They can be set after we copy their bytes.

alexanderbez · 2018-11-04T13:58:15Z

x/auth/account.go

+		Sequence:      acc.Sequence,
+	}
+
+	addr := make([]byte, len(acc.Address))


I would do the same thing for PubKey. See comment above.

alexanderbez · 2018-11-04T13:58:23Z

x/auth/account.go

@@ -121,6 +123,35 @@ func (acc *BaseAccount) SetSequence(seq int64) error {
 	return nil
 }

+// Implements sdk.Account.
+func (acc *BaseAccount) Clone() Account {
+	// given the fact PubKey and Address doesn't change,


I'd remove this comment.

alexanderbez · 2018-11-04T13:59:09Z

x/auth/mapper.go

+	maxCacheNumber = 10000
+)
+
+// accountLRUCache is a cache for the amino decoding of accounts. because we get and set accounts


Suggested change

// accountLRUCache is a cache for the amino decoding of accounts. because we get and set accounts

// accountLRUCache is a cache for the amino decoding of accounts. Because we get and set accounts

alexanderbez · 2018-11-04T13:59:41Z

x/auth/mapper.go

+func newAccountLRUCache(cap int) *accountLRUCache {
+	cache, err := lru.New(cap)
+	if err != nil {
+		panic(err)


Can we error wrap this? i.e. errors.Wrap(err, "failed to create account LRU cache").

alexanderbez · 2018-11-04T14:02:10Z

x/auth/mapper.go

+	return nil, false
+}
+
+func (cache *accountLRUCache) addAccount(marshalled string, acc Account) {


Suggested change

func (cache *accountLRUCache) addAccount(marshalled string, acc Account) {

func (cache *accountLRUCache) setAccount(marshalled string, acc Account) {

alexanderbez · 2018-11-04T14:02:42Z

x/auth/mapper.go

+	cache.cache.Add(marshalled, acc.Clone())
+}
+
+// validatorCache-key: validator amino bytes


typo and should probably be at the top of the file.

x/auth/mapper.go

yutianwu · 2018-11-06T07:31:58Z

thx for your comments, I am kind of busy this week. But I have a concern that if account data is too large, we may get a big key.

alexanderbez · 2018-11-06T12:11:40Z

Mhmm, good point. Hashing the key would also add overhead we don't want.

yutianwu · 2018-11-06T12:31:55Z

Ideally, the account cache should be like this. we cache account struct in memory (CheckAccountCache, DeliverAccountCache), they share the common cache AccountCache which also caches Account struct in memory.
When we commit a block, we will write the changes to AccountCache which will marshal account struct and save to disk.

CheckAccountCache and DeliverAccountCache should work like CacheMultiStore and we can commit the changes to AccountCache. And AccountCache uses a LRUCache for Accounts.

All the caches use address as key, so we do not have to worry about the large key.

Actually, it is kind of like simplified cache for Database and we do not need to care about replication delay between master and slave.

@alexanderbez what's your view?

alexanderbez · 2018-12-04T18:43:54Z

/cc @jaekwon

jaekwon · 2018-12-06T12:12:35Z

@yutianwu

How much time is this saving? When the IAVL store gets more full (e.g. 100s of millions of items), what is the relative improvement then?

Ideally, the account cache should be like this. we cache account struct in memory (CheckAccountCache, DeliverAccountCache), they share the common cache AccountCache which also caches Account struct in memory.

Some form of this is possible, but because CacheMultiStore etc already does most of this, instead it might make more sense to just find ways to make the encoding faster, which we will work on next year.

Alternatively, I think it might make sense to consider an extension to the storage system, where we have KVStore but also a ClonerStore or KVClonerStore which wraps a KVStore... and it would be set in the context somehow... Let me think more about this, it requires more thinking but I don't have much time for it atm.

jaekwon · 2018-12-06T12:13:09Z

Otherwise, I don't think we should merge this in yet without more information about how much time is actually being saved, given various store sizes.

jaekwon · 2018-12-09T07:35:26Z

Lets start designing an KeyObjectStore wrapper around the KVStore. This should help. How should we keep in touch? What project are you with and are you in our slack?

yutianwu · 2018-12-11T02:52:15Z

Lets start designing an KeyObjectStore wrapper around the KVStore. This should help. How should we keep in touch? What project are you with and are you in our slack?

@jaekwon sorry, recently I have been busy testing the performance and just get the result, I have submit the prototype of account cache which I used in our project and it doubled our throughput.

and the prototype implement the thought I described in the upper image. we can discuss the better implementation of the cache, but it surely help a lot. thx for you reply.

actually I am not in you slack, can you kindly invite me? my email is wzxingbupt#gmail#com

cwgoes · 2019-01-17T15:30:16Z

This is still a good idea but unfortunately quite out of date - @yutianwu are you planning to take this up again or should we close it?

yutianwu · 2019-01-18T07:56:52Z

This is still a good idea but unfortunately quite out of date - @yutianwu are you planning to take this up again or should we close it?

I'll close it ~

yutianwu requested a review from jaekwon as a code owner November 4, 2018 10:53

yutianwu changed the title ~~[R4R] x/auth: add lru cache for account~~ R4R: [x/auth] Add lru cache for account Nov 4, 2018

alexanderbez suggested changes Nov 4, 2018

View reviewed changes

alexanderbez added ready-for-review labels Nov 4, 2018

yutianwu changed the title ~~R4R: [x/auth] Add lru cache for account~~ WIP: [x/auth] Add lru cache for account Nov 12, 2018

fedekunze added wip and removed ready-for-review labels Nov 27, 2018

add account cache prototype

3f9cda1

yutianwu force-pushed the feature/cache_account branch from a09286f to 3f9cda1 Compare December 11, 2018 02:47

yutianwu requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners December 11, 2018 02:47

cwgoes added the will-close label Jan 17, 2019

yutianwu closed this Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: [x/auth] Add lru cache for account #2678

WIP: [x/auth] Add lru cache for account #2678

yutianwu commented Nov 4, 2018 •

edited

Loading

codecov bot commented Nov 4, 2018 •

edited

Loading

alexanderbez left a comment

alexanderbez Nov 4, 2018

ValarDragon Nov 4, 2018 •

edited

Loading

alexanderbez Nov 4, 2018

melekes Nov 7, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

yutianwu commented Nov 6, 2018

alexanderbez commented Nov 6, 2018

yutianwu commented Nov 6, 2018 •

edited

Loading

alexanderbez commented Dec 4, 2018

jaekwon commented Dec 6, 2018

jaekwon commented Dec 6, 2018

jaekwon commented Dec 9, 2018 •

edited

Loading

yutianwu commented Dec 11, 2018 •

edited

Loading

cwgoes commented Jan 17, 2019

yutianwu commented Jan 18, 2019

	// Implements sdk.Account.
	// Clone creates a deep copy of a BaseAccount. It implements the sdk.Account interface.

	// accountLRUCache is a cache for the amino decoding of accounts. because we get and set accounts
	// accountLRUCache is a cache for the amino decoding of accounts. Because we get and set accounts

	func (cache *accountLRUCache) addAccount(marshalled string, acc Account) {
	func (cache *accountLRUCache) setAccount(marshalled string, acc Account) {

WIP: [x/auth] Add lru cache for account #2678

WIP: [x/auth] Add lru cache for account #2678

Conversation

yutianwu commented Nov 4, 2018 • edited Loading

codecov bot commented Nov 4, 2018 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Nov 4, 2018 • edited Loading

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

Choose a reason for hiding this comment

yutianwu commented Nov 6, 2018

alexanderbez commented Nov 6, 2018

yutianwu commented Nov 6, 2018 • edited Loading

alexanderbez commented Dec 4, 2018

jaekwon commented Dec 6, 2018

jaekwon commented Dec 6, 2018

jaekwon commented Dec 9, 2018 • edited Loading

yutianwu commented Dec 11, 2018 • edited Loading

cwgoes commented Jan 17, 2019

yutianwu commented Jan 18, 2019

yutianwu commented Nov 4, 2018 •

edited

Loading

codecov bot commented Nov 4, 2018 •

edited

Loading

ValarDragon Nov 4, 2018 •

edited

Loading

yutianwu commented Nov 6, 2018 •

edited

Loading

jaekwon commented Dec 9, 2018 •

edited

Loading

yutianwu commented Dec 11, 2018 •

edited

Loading