Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Remove unneeded locking #499

Merged
merged 12 commits into from
Mar 11, 2016
Merged

Remove unneeded locking #499

merged 12 commits into from
Mar 11, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Feb 22, 2016

Reduced lock contention a bit.
There is no global lock on the blockchain anymore, instead there is a lock on the insert_block function that guarantees that all updates are atomic.
Reads are are also atomic: database changes are committed with the transaction. Most of the caches that are addressed by block hash can be updated individually. The only exceptions are best_block, block_hashes and transaction_addresses. These are updated alltogether.
Block queue has been optimized to use more fine grained locking.
Global lock on the block queue has been removed. It was not needed in the first place since the module is fully reenterable.

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Feb 22, 2016
@arkpar arkpar closed this Feb 23, 2016
@arkpar arkpar reopened this Feb 23, 2016
@arkpar arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 26, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 29, 2016
@@ -753,8 +770,16 @@ impl BlockChain {

// TODO: handle block_hashes properly.
block_hashes.clear();

blocks.shrink_to_fit();
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing something from a HashMap does not guarantee the memory will be deallocated. We use heapsize to measure cache size on line 782. The condition there would always be false after max size is reached for the first time.

@NikVolf NikVolf added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 1, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 2, 2016
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 2, 2016
@arkpar arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Mar 2, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A0-pleasereview 🤓 Pull request needs code review. labels Mar 8, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 10, 2016
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 11, 2016
debris added a commit that referenced this pull request Mar 11, 2016
@debris debris merged commit 8d6ea3a into master Mar 11, 2016
@arkpar arkpar deleted the thread branch March 15, 2016 13:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants