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

Speed up "Synchronizing DAO" by ~30% #5484

Merged
merged 7 commits into from
May 17, 2021

Conversation

cd2357
Copy link
Contributor

@cd2357 cd2357 commented May 11, 2021

BlockParser.parseBlock() normally takes about 300ms per block.

One part of it is checking if the new block hash is known, which was done by string-comparing it to the hashes of all known blocks. In my case, this was about 110 000 string-comparisons per BlockParser.parseBlock() call.

This commit adds a block hash cache in the DaoState and replaces the hash string-comparisons by one HashSet lookup.

With this change, BlockParser.parseBlock() takes around 200ms on my setup.

Measure the entire parseBlock() duration, not just the substep of parsing transactions.
Improve isBlockHashKnown() by about 100ms per call by caching the hashes of the known blocks.
Method is duplicate of isBlockHashKnown().
Improve block lookup by height by introducing an index, instead of iterating through all blocks and comparing the height.
@ripcurlx
Copy link
Contributor

As by definition this needs an ACK by @ManfredKarrer to get merged.

@cd2357 cd2357 changed the title Speed up "Synchronizing DAO" Speed up "Synchronizing DAO" by ~30% May 11, 2021
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

@cd2357 Thanks for working on that.
There is missing the chache update when a new block is added in onNewBlockWithEmptyTxs.
Maybe its better to create explicit methods for daoState.getBlocks().addAll and daoState.getBlocks().add in DaoStateService.

core/src/main/java/bisq/core/dao/state/model/DaoState.java Outdated Show resolved Hide resolved
@ripcurlx
Copy link
Contributor

@chimp1984 Thanks for doing a review as well! But still we'll need in the end the ACK by @ManfredKarrer as well.

@chimp1984
Copy link
Contributor

chimp1984 commented May 12, 2021

@chimp1984 Thanks for doing a review as well! But still we'll need in the end the ACK by @ManfredKarrer as well.

Just to be clear, it is a NACK at current state. Cache does not get updated at new blocks.

Update the two new indices on every known write to the list, namely addBlock() and addBlocks(). Restrict ability to modify the blocks list outside of these methods by overwriting the getBlocks() getter such that it returns an unmodifiable list.
@cd2357
Copy link
Contributor Author

cd2357 commented May 15, 2021

Done, thanks for the speedy review.

There is missing the chache update when a new block is added in onNewBlockWithEmptyTxs.
Maybe its better to create explicit methods for daoState.getBlocks().addAll and daoState.getBlocks().add in DaoStateService.

Good point. I restricted the adding of blocks to two new methods, addBlock and addBlocks. They make sure to update all the indices as well. Also made sure the DaoState.blocks list is not exposed directly anymore, but as an unmodifiable list, so no direct modifications are possible.

chimp1984
chimp1984 previously approved these changes May 17, 2021
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

Beside the small naming issue utACK

core/src/main/java/bisq/core/dao/state/model/DaoState.java Outdated Show resolved Hide resolved
Rename blocksByHash because it is not actually indexing blocks by hashes, instead it's a cache of known hashes.
@ripcurlx
Copy link
Contributor

So only the ACK by @ManfredKarrer is finally missing 😄 👍

Copy link
Member

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK based on #5484 (review)

@ripcurlx ripcurlx added this to the v1.6.5 milestone May 17, 2021
@ripcurlx ripcurlx merged commit 08320d8 into bisq-network:master May 17, 2021
cd2357 added a commit to cd2357/bisq that referenced this pull request Jul 4, 2021
…dao"

This reverts commit 08320d8, reversing
changes made to ee6ccc4.
cd2357 added a commit to cd2357/bisq that referenced this pull request Jul 4, 2021
…dao"

This reverts commit 08320d8, reversing
changes made to ee6ccc4.
@cd2357 cd2357 deleted the faster-sync-dao branch November 6, 2021 10:38
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.

4 participants