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

[poc]blockchain: Load blockchain nodes in parallel #918

Closed
wants to merge 13 commits into from

Conversation

matheusd
Copy link
Member

This adds a new function on the blockchain to load all nodes from the block database during startup at once, in a parallelized fashion.

It also adds an index to track the SpentTicketsInBlock for every block on the database. This speeds up startup time by removing the need to load and decode all blocks in order to extract ticket information needed to calculate the stake difficulty.

Closes #913
Requires #915

@matheusd
Copy link
Member Author

matheusd commented Nov 14, 2017

This is being sent as separate from #915 because involves deeper changes in the startup procedure.

Difference in time between versions (in seconds, on my machine, with 6 cores running the parallelized version):

| variation                          | mainnet | testnet |
| vanilla (#e145193)                 |     40  |      80 |
| w/ ticketsInBlockInfo (#915)       |     26  |      47 |
| w/ index + serial LoadAllBlocks    |     40  |      36 |
| w/ index + parallel LoadAllBlocks  |     17  |      17 |
| same as above + custom encoding    |     11  |       8 |

Marked as [poc] for the moment, because I need feedback on a few issues to continue this line of work. I expect the code itself to be heavily changed and refactored. I'm sending the work as a single function (LoadAllBlocksByBatchHeader) to make it easier to reason about, but expect to refactor into many subfunctions/different packages.

Specific stuff I need feedback on:

  1. Is it ok to add a new index to store the SpentTicketsInBlock for every block, to speed up the startup time?
  2. Should this index be entirely self-contained (as in the poc) or should I add it as an extra index (in the blockchain/indexers package)?
  3. Should I add a config flag to enable/disable usage of this index?
  4. Is it ok to load the nodes in a parallelized fashion?
  5. Should I add a config flag to specify how many workers to use in the parallelized version?

@matheusd
Copy link
Member Author

FYI, switching from gson to a custom binary encoding of SpentTicketsInBlock further reduces the time by about 50%. Decrediton on testnet for development loads almost instantly now.

@dajohi
Copy link
Member

dajohi commented Nov 16, 2017

tests are failing on travis...

@matheusd
Copy link
Member Author

Ok, I fixed the travis problem, but right now I'm not looking for a thorough review of the code, just really an answer whether the approach of adding a new index to speed up the startup time would be acceptable or not.

@jpz
Copy link
Contributor

jpz commented Nov 17, 2017

Hi - how are the before/after tests set up? Are you looking at specific markers in the logs? I'm happy to take a look.

@matheusd
Copy link
Member Author

Yeah, look at the logs. Start a vanilla dcrd, it will hang after "Chain State: Height...". Without this patch, the next line in the logs will be "RPC server listening on...".

Check the difference in load time on current master vs this patch applied. The chain should be synced to tip before these tests.

@davecgh
Copy link
Member

davecgh commented Nov 29, 2017

Thanks for the PR and I'll have to take a closer look at this before commenting definitively, but a quick look at this says it's going in the wrong direction as compared to upstream.

The upstream code has significantly sped up operation by keep all headers in memory and, more importantly as it pertains to this, reworking the way the headers are stored to and loaded from the database. This seems to completely ignore that work and go in a direction that would be quite difficult to reconcile with that work. What I'm concerned about is making it a lot harder to get the significantly better upstream code merged due to ignoring it and just going in a different direction here.

I should also note that part of the challenge for Decred is that it has some additional requirements in terms of needing pruneable stake nodes, so there will need to be some additional work done in that regard.

@matheusd
Copy link
Member Author

Thanks for the update! I'm not as familiar with the upstream code and practices compared to dcrd, so can you tell me whether that is an ongoing work tracked by some issue/PR/discussion somewhere that I can follow?

What would be the appropriate way to make these sorts of contributions in the future? Validate they are a problem in btcd and send them there first?

@davecgh
Copy link
Member

davecgh commented Nov 30, 2017

The relevant PRs start stomewhere around btcsuite/btcd#913. You'll notice there are a lot of them related to blockchain, chainviews, headers, etc.

@davecgh
Copy link
Member

davecgh commented Jan 29, 2018

See the recent work in #988, #989, #990 which is starting to move the code closer to the upstream code.

@matheusd
Copy link
Member Author

👍
Once all these new refactoring PRs are in, I'll make another try.

@davecgh
Copy link
Member

davecgh commented Jan 29, 2018

Cool, thanks. Also, for reference, btcsuite/btcd#919, btcsuite/btcd#1010, and btcsuite/btcd#1014 are pretty significant changes that play into this and will be making their way into dcrd.

@matheusd
Copy link
Member Author

superseded

@matheusd matheusd closed this Feb 22, 2018
@matheusd matheusd deleted the load-node-index-parallel branch November 25, 2020 21:44
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