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

Return Arc instead of Rc from the iterators to make them Send #185

Merged
merged 7 commits into from
Feb 24, 2023

Conversation

koute
Copy link
Contributor

@koute koute commented Feb 23, 2023

This PR replaces the Rc that's returned by the iterators with an Arc.

I need this to make the iterators Send, which I need for further performance improvements in substrate.

@cheme
Copy link
Contributor

cheme commented Feb 23, 2023

Does TrieDBNodeIterator need to be Send (if not, could also make send the other needed iterators as long as they cannot leek rc (the internal triedbnodeiterator))?

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

A good old is_sync test maybe?

Copy link

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@koute
Copy link
Contributor Author

koute commented Feb 24, 2023

Does TrieDBNodeIterator need to be Send (if not, could also make send the other needed iterators as long as they cannot leek rc (the internal triedbnodeiterator))?

I just need TrieDBRawIterator to be Send, but since TrieDBNodeIterator is a thin wrapper around it it'll also automatically become Send. There are no other Rcs anywhere left so I think everything now will be Send anyway? actually, sorry, I had a brain fart, indeed TrieDBNodeIterator is not Send since there's a RefCell inside of TrieDB. We could replace the RefCell with a Mutex or something to make it Send (or perhaps do a more thorough refactoring), but for now I'll just leave it as is since that'd complicate things and I don't really need it.

A good old is_sync test maybe?

Good idea. I'll add it.

@koute koute merged commit 5d011b2 into paritytech:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants