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

Add Reference Copying #5245

Merged
merged 68 commits into from
Apr 19, 2020
Merged

Add Reference Copying #5245

merged 68 commits into from
Apr 19, 2020

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Mar 30, 2020

This allows us to copy references to the objects instead of the actual objects. Doing this will reduce the required amount of memory when copying state fields. This is gated behind a feature flag so this doesn't affect production nodes in case of a bug.

  • Add reference copying for fields
  • Add feature flag
  • Change method signature of ApplyToEveryValidator to take in two different functions
    so as to prevent unecessary clones.
  • Use memory pools correctly for tries.
  • Tests to ensure mutation is not possible
    • Validators (update + apply to all validators methods)
    • Block Roots
    • State Roots
    • Pending and Current Attestations
    • RANDAO

@nisdas nisdas added the Ready For Review A pull request ready for code review label Mar 30, 2020
@nisdas nisdas closed this Mar 30, 2020
@nisdas nisdas reopened this Mar 30, 2020
beacon-chain/state/setters.go Outdated Show resolved Hide resolved
shared/memorypool/memorypool.go Outdated Show resolved Hide resolved
shared/featureconfig/flags.go Outdated Show resolved Hide resolved
shared/featureconfig/config.go Outdated Show resolved Hide resolved
shared/featureconfig/config.go Outdated Show resolved Hide resolved
@prestonvanloon
Copy link
Member

Discussed offline, please add some tests for these methods to validate and ensure mutation is not possible using this library. Especially the pointer slices like validators and pending attestations.

@stale
Copy link

stale bot commented Apr 7, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale There hasn't been any activity here in some time... label Apr 7, 2020
@nisdas nisdas removed the Stale There hasn't been any activity here in some time... label Apr 11, 2020
@farazdagi farazdagi added In Progress and removed Ready For Review A pull request ready for code review labels Apr 11, 2020
@nisdas nisdas mentioned this pull request Apr 11, 2020
@nisdas nisdas requested a review from a team as a code owner April 13, 2020 11:42
@farazdagi farazdagi marked this pull request as draft April 13, 2020 11:43
@prylabs-bulldozer prylabs-bulldozer bot merged commit 626b3e0 into master Apr 19, 2020
@delete-merged-branch delete-merged-branch bot deleted the addBetterCopying branch April 19, 2020 13:57
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

5 participants