Form tipsets #875
Form tipsets #875
Conversation
| @@ -102,6 +102,9 @@ pub struct ChainStore<DB> { | |||
|
|
|||
| /// Used as a cache for tipset lookbacks. | |||
| chain_index: ChainIndex<DB>, | |||
|
|
|||
| /// Tracks blocks for the purpose of forming tipsets. | |||
| tipset_tracker: RwLock<TipsetTracker<DB>>, | |||
There was a problem hiding this comment.
I think it would be best to only wrap the type that interior mutability with the mutex, instead of wrapping the whole tracker type. Also, what are your thoughts on using something like https://github.com/xacrimon/dashmap for a threadsafe map, instead of using mutexes here?
There was a problem hiding this comment.
To me this seemed like the more natural place for the lock because it's only in ChainStore that the need for concurrent writing arises, TipsetTracker in isolation doesn't really care about any of this. e.g. if at some point we hypothetically need to use this tracker without the need for concurrency, locks would be unnecessary. Is that fair?
dashmap is probably faster here, though I don't know if it also optimizes for many concurrent readers like RwLock does? I'll leave a note about possibly improving this, at the moment I'm unsure about pulling in a dependency without knowing how much of a speedup it gives us.
There was a problem hiding this comment.
Well, I still don't see the reason why the mutex scope shouldn't be reduced, since the db never needs interior mutability. After reading the other parts a bit more I do see that the way it's setup forces a lock to be held for the whole function, when Cids are very cheap to clone. I just see the issue where each add, which happens after each block is validated, holds the lock while it performs io with the database during the function.
I don't think it's an issue with the current usage, but just thinking ahead to how this is setup. And yes we currently need this tracker to be used concurrently, as there will be multiple workers doing state transitions. That's why these types are setup to be threadsafe. We just have the default workers set to 1 for testing until everything is functional as there was some redundant overhead and made it harder to debug
| persist_objects(self.blockstore(), ts.blocks())?; | ||
| // TODO determine if expanded tipset is required; see https://github.com/filecoin-project/lotus/blob/testnet/3/chain/store/store.go#L236 | ||
| self.update_heaviest(ts).await?; | ||
| let expanded = self.expand_tipset(ts.min_ticket_block().clone()).await?; |
There was a problem hiding this comment.
this seems pretty redundant, if the heaviest is based on the expanded, why you need to persist the tipset being passed in? Shouldn't this be able to use the other blocks in this passed in tipset as well when expanding?
| @@ -102,6 +102,9 @@ pub struct ChainStore<DB> { | |||
|
|
|||
| /// Used as a cache for tipset lookbacks. | |||
| chain_index: ChainIndex<DB>, | |||
|
|
|||
| /// Tracks blocks for the purpose of forming tipsets. | |||
| tipset_tracker: RwLock<TipsetTracker<DB>>, | |||
There was a problem hiding this comment.
Well, I still don't see the reason why the mutex scope shouldn't be reduced, since the db never needs interior mutability. After reading the other parts a bit more I do see that the way it's setup forces a lock to be held for the whole function, when Cids are very cheap to clone. I just see the issue where each add, which happens after each block is validated, holds the lock while it performs io with the database during the function.
I don't think it's an issue with the current usage, but just thinking ahead to how this is setup. And yes we currently need this tracker to be used concurrently, as there will be multiple workers doing state transitions. That's why these types are setup to be threadsafe. We just have the default workers set to 1 for testing until everything is functional as there was some redundant overhead and made it harder to debug
Summary of changes
Changes introduced in this pull request:
TipsetTrackertype that tracks blocks and can form tipsets from themReference issue to close (if applicable)
Closes #758
Other information and links