Skip to content
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

Change slasher cache to LRU cache #5037

Merged
merged 12 commits into from Mar 8, 2020

Conversation

0xKiwi
Copy link
Contributor

@0xKiwi 0xKiwi commented Mar 8, 2020

Part of #4836

This PR changes the risettro LFU cache to a LRU cache which better matches our use case.

@@ -0,0 +1,73 @@
package cache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps make the name cache.go more specific, when person B comes in and implements a new cache for other functionality, they don't have to worry about changing both

@codecov
Copy link

codecov bot commented Mar 8, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@962fe85). Click here to learn what that means.
The diff coverage is 60.86%.

@@            Coverage Diff            @@
##             master    #5037   +/-   ##
=========================================
  Coverage          ?   14.87%           
=========================================
  Files             ?       98           
  Lines             ?     7389           
  Branches          ?        0           
=========================================
  Hits              ?     1099           
  Misses            ?     6160           
  Partials          ?      130

@0xKiwi 0xKiwi changed the title [WIP] Change slasher cache to LRU cache Change slasher cache to LRU cache Mar 8, 2020
@0xKiwi 0xKiwi added the Ready For Review A pull request ready for code review label Mar 8, 2020

// SaveAllToDB removes all keys from the SpanCache.
func (c *EpochSpansCache) SaveAllToDB() {
//c.cache.Keys()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, was working on that to replace the onEvict risettro has. Will fill it in.

}

// Delete removes an epoch from the cache and returns if it existed or not.
func (c *EpochSpansCache) Delete(epoch uint64) (present bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why (present bool)? instead of bool?, doesn't look like the standard convention to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be confusing what the return bools mean without context, but it's mentioned in the comment too

@@ -83,11 +83,11 @@ func (ds *Service) Start() {

// The detection service runs detection on all historical
// chain data since genesis.
go ds.detectHistoricalChainData(ds.ctx)
//go ds.detectHistoricalChainData(ds.ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why comment these out?

Copy link
Contributor Author

@0xKiwi 0xKiwi Mar 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out in Raul's PR. Not functioning at the moment and gets in the way of testing live detection.

@0xKiwi 0xKiwi removed the Ready For Review A pull request ready for code review label Mar 8, 2020
@0xKiwi 0xKiwi added the Ready For Review A pull request ready for code review label Mar 8, 2020
@0xKiwi 0xKiwi merged commit d4cd51f into prysmaticlabs:master Mar 8, 2020
Copy link
Contributor

@shayzluf shayzluf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test TestValidatorSpanMap_SaveOnEvict
doesn't test eviction any more as those parameters
setupDBDiffCacheSize(t testing.TB, cacheItems int64, maxCacheSize int64) *Store {
aren't being used

@0xKiwi 0xKiwi deleted the slasher-change-cache branch March 15, 2020 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants