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

Refactor SyncManager to have ownership over tipsets #238

Merged
merged 3 commits into from
Feb 21, 2020

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • I wrote the syncmanager assuming that it would just use references to the tipsets kept in memory (optimistic thinking, I know) but in fact how it needs to happen is tipsets will be pulled from the network and kept in memory only for the sync manager. Because of this, the tipsets need to be heap allocated using an Rc (https://doc.rust-lang.org/std/rc/struct.Rc.html) and clone references to it to use in the sync manager.

Don't be worried about the clone, it only creates a new pointer to the heap allocated tipset and increments the reference count. Also don't need to use the non-associative Rc::clone(&other) in my usages here because the type in enforced by the function usage, but be careful when cloning a tipset into the sync manager not to create a duplicate of it.

Reference issue to close (if applicable)

Closes

Other information and links

Copy link
Contributor

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

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

Should the same approach used here using Rc be applied to the heaviest tipset in the ChainStore?

@austinabell
Copy link
Contributor Author

Should the same approach used here using Rc be applied to the heaviest tipset in the ChainStore?

I have changed it on my local branch working changes, but depending on usage that tipset may only need to be owned only by the chain store. (Will know for sure once I finish implementing something)

@austinabell austinabell merged commit faa7138 into master Feb 21, 2020
@austinabell austinabell deleted the austin/syncman/rew branch February 21, 2020 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants