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.

Sign up for GitHub

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

Jump to bottom

Optimize List Validator Assignments #4456

Merged
merged 12 commits into from Jan 9, 2020
Merged

Optimize List Validator Assignments #4456

merged 12 commits into from Jan 9, 2020

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Jan 8, 2020

  • Only retrieves the committee cache once, to prevent redundant computation
  • Retrieves archival data from epoch only once per call

@nisdas nisdas added the Ready For Review A pull request ready for code review label Jan 8, 2020
@codecov
Copy link

codecov bot commented Jan 8, 2020 •
edited

Codecov Report

Merging #4456 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4456   +/-   ##
=======================================
  Coverage   18.17%   18.17%           
=======================================
  Files         193      193           
  Lines       12822    12822           
=======================================
  Hits         2330     2330           
  Misses      10206    10206           
  Partials      286      286

terencechain
terencechain requested changes Jan 8, 2020
View reviewed changes
beacon-chain/rpc/beacon/assignments.go Outdated
archivedBalances := []uint64{}

if shouldFetchFromArchive {
archivedInfo, err = bs.BeaconDB.ArchivedCommitteeInfo(ctx, requestedEpoch)
Copy link
Member

Choose a reason for hiding this comment

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

I know this is not new code, but can you refactor everything within shouldFetchFromArchive into its function?

if shouldFetchFromArchive {
        bs.archivedCommitteeAssignment(ctx, requestedEpoch)
        ...
} else {
	committeeAssignments, proposerIndexToSlot, err = helpers.CommitteeAssignments(headState, requestedEpoch)
	if err != nil {
		return nil, status.Errorf(codes.Internal, "Could not compute committee assignments: %v", err)
	}
}

prestonvanloon
prestonvanloon reviewed Jan 8, 2020
View reviewed changes
beacon-chain/rpc/beacon/assignments.go
@@ -157,15 +171,12 @@ func (bs *Server) ListValidatorAssignments(
res = append(res, assign)
continue
}
committee, committeeIndex, attesterSlot, proposerSlot, err := helpers.CommitteeAssignment(headState, requestedEpoch, index)
Copy link
Member

Choose a reason for hiding this comment

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

Yikes, this was the really slow one!

beacon-chain/rpc/beacon/assignments.go Outdated
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not retrieve assignment for validator %d: %v", index, err)
}
comAssingnment := committeeAssignments[index]
Copy link
Member

Choose a reason for hiding this comment

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

Typo here

terencechain
terencechain approved these changes Jan 9, 2020
View reviewed changes
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.

Thank you for the changes!

@prylabs-bulldozer prylabs-bulldozer bot merged commit 699e1c8 into master Jan 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the listValidatorAssingments branch January 9, 2020 05:07
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
@nisdas @cryptomental
Optimize List Validator Assignments (prysmaticlabs#4456)
70a6a9d 
* optimize
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* terence's and preston's review
* Merge branch 'listValidatorAssingments' of https://github.com/prysmaticlabs/geth-sharding into listValidatorAssingments
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
@nisdas @cryptomental
Optimize List Validator Assignments (prysmaticlabs#4456)
e72de60 
* optimize
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* Merge refs/heads/master into listValidatorAssingments
* terence's and preston's review
* Merge branch 'listValidatorAssingments' of https://github.com/prysmaticlabs/geth-sharding into listValidatorAssingments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prestonvanloon prestonvanloon prestonvanloon approved these changes

@terencechain terencechain terencechain approved these changes

Assignees
No one assigned
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nisdas @prestonvanloon @terencechain