[Ledger] Replace LRU cache with a FIFO queue (circular buffer) #2893
Conversation
ramtinms
changed the title
There was a problem hiding this comment.
Great work and amazing how insanely fast you created this PR, right after talking about it!
Some suggestions:
- Replace
sync.Mutexwithsync.RWMutexin buffer. - Add
forest.PurgeCacheExcept()test in forest_test.go. - Replace references of LRU cache to FIFO queue in docs in ledger.go and forest.go.
First item is in the code comments but 2 and 3 are just here because it is unmodified by this PR.
Also, maybe rename Buffer to be more specific if you'd like. I don't have a better name for it 😆
ledger/complete/mtrie/trieCache.go
Outdated
|
|
||
| tries := make([]*trie.MTrie, tc.count) | ||
|
|
||
| if tc.isFull() { |
There was a problem hiding this comment.
just a random though: if we used a double-linked list, would the logic here be a bit simpler?
There was a problem hiding this comment.
array is much cheaper than link list
There was a problem hiding this comment.
agree! that said, given that we use map for O(1) access it should not affect perf too much (esp. given how infrequently we should update the forest.)
There was a problem hiding this comment.
there is going to be a follow up PR for improvements, going to consider your suggestions in the PR.
| @@ -145,7 +145,6 @@ func BenchmarkLoadCheckpointAndWALs(b *testing.B) { | |||
| return err | |||
| }, | |||
| func(rootHash ledger.RootHash) error { | |||
| forest.RemoveTrie(rootHash) | |||
There was a problem hiding this comment.
With removal of removal, maybe we can get out of it completely, remove WALDelete (and WALUpdate since now we have only one operation, and all related code
There was a problem hiding this comment.
I think we need it for reading old WALs when we spork, we could get rid of it after next spork.
| // test deletion | ||
| s := led2.ForestSize() | ||
| assert.Equal(t, s, size) | ||
|
|
There was a problem hiding this comment.
We don't do removals any more so this part is not relevant anymore
| defer tc.lock.RUnlock() | ||
|
|
||
| if tc.count == 0 { | ||
| return nil |
There was a problem hiding this comment.
suggestion, if you return an empty slice, it would make the job of caller easier. they won't have to check for nil anymore
There was a problem hiding this comment.
hey @taabodim we are going to have a follow up PR for improvements, going to include your suggestions in that PR.
This PR
But, Why?
The initial decision to use an LRU cache had roots in another type of design considered for the ledger. ideally, the ledger should only care about purging the oldest trie added to the buffer and get trie calls having any impact on which trie is going to be evicted. This would also make the checkpointer and forest to be in harmony with how they deal with trie updates.
Why LRU cache was a bad idea? in the LRU version scripts would change the order of tries kept in the cache, and this could result in issues loosing the head of the trie we need to build blocks. in other words, one might send a lot of scripts targeting older tries with the hope to purge a necessary trie in memory and halt the execution.