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

Bound Initial Sync Cache Size #4844

Merged
merged 83 commits into from Feb 18, 2020
Merged

Bound Initial Sync Cache Size #4844

merged 83 commits into from Feb 18, 2020

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Feb 12, 2020

Resolves #4813

  • Only Cache Boundary States instead of all unfinalized states.
  • Enforce Cache Limit so that states are flushed from memory to disk, in the
    event caches are full.
  • Filter out boundary candidate states at the start of each epoch.
  • Use memory pool for Randao mixes field.
  • Add flag to toggle with garbage collector.

@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

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

@@            Coverage Diff            @@
##             master    #4844   +/-   ##
=========================================
  Coverage          ?   43.48%           
=========================================
  Files             ?      227           
  Lines             ?    17016           
  Branches          ?        0           
=========================================
  Hits              ?     7399           
  Misses            ?     8366           
  Partials          ?     1251

@rauljordan rauljordan marked this pull request as ready for review February 12, 2020 20:08
rauljordan
rauljordan previously approved these changes Feb 12, 2020
if featureconfig.Get().InitSyncCacheState {
numOfStates := len(s.initSyncState)
if numOfStates > initialSyncCacheSize {
stateSlice := make([][32]byte, 0, numOfStates)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please refactor this into its own function?

numOfStates := len(s.initSyncState)
if numOfStates > initialSyncCacheSize {
stateSlice := make([][32]byte, 0, numOfStates)
for rt := range s.initSyncState {
Copy link
Member

Choose a reason for hiding this comment

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

Tests?

nisdas and others added 8 commits February 18, 2020 08:01
@@ -16,3 +22,21 @@ func New(db db.NoHeadAccessDatabase) *State {
beaconDB: db,
}
}

// GenerateState generates a state from the provided pre-state till the provided block.
func (s *State) GenerateState(ctx context.Context, preState *stateTrie.BeaconState, endBlock *ethpb.SignedBeaconBlock) (*stateTrie.BeaconState, error) {
Copy link
Member

Choose a reason for hiding this comment

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

can you put this back in block chain service.go, you can combine it with: https://github.com/prysmaticlabs/prysm/pull/4844/files#diff-c2ffc98dd863a51dc11c4843367b82b6R160

this API pattern with preState and endBlock seem to be only serving block chain service specific use case, because of that, we shouldn't put it here.

I'll export loadBlocks and replayBlocks via an interface for you

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't something like this also be useful for other services, ex: Sync,etc ? You can generate a new state from a provided pre-state

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but we don't know what the API is for that yet. Example, it may want to use endSlot instead of endBlock. I think until sync actually needs it / uses it. It's better to constraint this within the block chains service.
Or else it may introduce disparity where chain service uses one API to generate state and sync service uses another API to generate state and both APIs reside in stategen service

if endBlock == nil {
return nil, errors.New("provided block root does not have block saved in the db")
}
stGen := stategen.New(s.beaconDB)
Copy link
Member

Choose a reason for hiding this comment

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

can you initialize this properly in node.go and pass it down to the service struct so stGen is part of *Service?

rauljordan
rauljordan previously approved these changes Feb 18, 2020
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks @nisdas

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Lgtm. Thank you for addressing the feedbacks

@rauljordan rauljordan merged commit 655f57e into master Feb 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the OOMFix branch February 18, 2020 21:11
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
* bound initial sync

* fix lint

* Revert "Better block attestation inclusion (prysmaticlabs#4838)"

This reverts commit 090d962.

* add memory pool

* more fixes

* revert changes

* add hack

* revert hack

* push halving

* bring back hack

* increase cache size

* more fixes

* more changes

* new fixes

* add test

* add reverse test

* more tests and clean up

* add helper

* more cleanup and tests

* fix test

* remove code

* set gc percent flag

* lint

* lint

* Fix comment formatting

* Fix some formatting

* inverse if statement

* remove debug log

* Apply suggestions from code review

Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>

* Update beacon-chain/state/getters.go

Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>

* Update beacon-chain/db/kv/state.go

* integrate state generator

* gaz

* fixes

* terence's review

* reduce bound further

* fix test

* separate into new files

* gaz

* mod build file

* add test

* revert changes

* fix test

* Update beacon-chain/core/helpers/slot_epoch.go

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

* handle edge case

* add back test

* fix test again

* handle edge case

* Update beacon-chain/blockchain/init_sync_process_block.go

* Update beacon-chain/blockchain/init_sync_process_block.go

* Update beacon-chain/stategen/service_test.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* Update beacon-chain/blockchain/init_sync_process_block.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* Update beacon-chain/stategen/service.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* Update beacon-chain/stategen/service.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* raul's review

* raul's review

* fix refs

* terence's review

* one more fix

* Update beacon-chain/blockchain/init_sync_process_block.go

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com>
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
* bound initial sync

* fix lint

* Revert "Better block attestation inclusion (prysmaticlabs#4838)"

This reverts commit 090d962.

* add memory pool

* more fixes

* revert changes

* add hack

* revert hack

* push halving

* bring back hack

* increase cache size

* more fixes

* more changes

* new fixes

* add test

* add reverse test

* more tests and clean up

* add helper

* more cleanup and tests

* fix test

* remove code

* set gc percent flag

* lint

* lint

* Fix comment formatting

* Fix some formatting

* inverse if statement

* remove debug log

* Apply suggestions from code review

Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>

* Update beacon-chain/state/getters.go

Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>

* Update beacon-chain/db/kv/state.go

* integrate state generator

* gaz

* fixes

* terence's review

* reduce bound further

* fix test

* separate into new files

* gaz

* mod build file

* add test

* revert changes

* fix test

* Update beacon-chain/core/helpers/slot_epoch.go

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

* handle edge case

* add back test

* fix test again

* handle edge case

* Update beacon-chain/blockchain/init_sync_process_block.go

* Update beacon-chain/blockchain/init_sync_process_block.go

* Update beacon-chain/stategen/service_test.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* Update beacon-chain/blockchain/init_sync_process_block.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* Update beacon-chain/stategen/service.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* Update beacon-chain/stategen/service.go

Co-Authored-By: Raul Jordan <raul@prysmaticlabs.com>

* raul's review

* raul's review

* fix refs

* terence's review

* one more fix

* Update beacon-chain/blockchain/init_sync_process_block.go

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com>
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>
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.

Initial Sync Cache State Leads to OOMs
5 participants