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

Boost PreloadAsync() #420

Merged
merged 2 commits into from
Aug 27, 2019
Merged

Boost PreloadAsync() #420

merged 2 commits into from
Aug 27, 2019

Conversation

longfin
Copy link
Member

@longfin longfin commented Aug 9, 2019

This patch enhances the performance of Swarm.PreloadAsync() using two tricks as below.

  • Instead of using RecentStates.StateReferences directly, PreloadAsync became to translate this message as Dictionary<HashDigest<SHA256>, ISet<Address>> to reduce file I/O.
    • To accomplish this, IStore.StoreStateReference became to take blockHash and blockIndex, not whole Block.
  • PreloadAsync() are currently fork its chain to prevent corruption by unexpected termination. this patch reduces its file I/O by IStore.ForkIndexes()

@longfin longfin self-assigned this Aug 12, 2019
@longfin longfin force-pushed the feature/boost-ibd branch 7 times, most recently from 4049d43 to 00197ca Compare August 19, 2019 09:16
@longfin longfin force-pushed the feature/boost-ibd branch 3 times, most recently from 13b17c5 to f08577b Compare August 21, 2019 06:08
@longfin longfin requested review from dahlia, earlbread, moreal and limebell and removed request for dahlia August 21, 2019 06:20
@longfin longfin marked this pull request as ready for review August 21, 2019 06:21
earlbread
earlbread previously approved these changes Aug 21, 2019
@longfin
Copy link
Member Author

longfin commented Aug 21, 2019

I've rebased. PTAL.

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #420 into master will increase coverage by 0.03%.
The diff coverage is 93.82%.

@@            Coverage Diff            @@
##           master    #420      +/-   ##
=========================================
+ Coverage   87.97%     88%   +0.03%     
=========================================
  Files         195     195              
  Lines       13675   13729      +54     
=========================================
+ Hits        12030   12082      +52     
- Misses       1332    1334       +2     
  Partials      313     313
Impacted Files Coverage Δ
Libplanet/Store/StoreExtension.cs 95.55% <ø> (ø) ⬆️
Libplanet/Net/StateReferenceDownloadState.cs 0% <ø> (ø) ⬆️
Libplanet/Store/BaseStore.cs 81.25% <ø> (ø) ⬆️
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.4% <100%> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 94.53% <100%> (-0.01%) ⬇️
Libplanet.Tests/Store/StoreTest.cs 98.24% <100%> (+0.07%) ⬆️
Libplanet/Store/LiteDBStore.cs 89.33% <100%> (+0.16%) ⬆️
Libplanet.Tests/Store/StoreExtensionTest.cs 100% <100%> (ø) ⬆️
Libplanet.Tests/Store/StoreTracker.cs 47.85% <33.33%> (-1.41%) ⬇️
Libplanet/Net/Swarm.cs 80.23% <93.75%> (+0.42%) ⬆️
... and 2 more

earlbread
earlbread previously approved these changes Aug 21, 2019
moreal
moreal previously approved these changes Aug 22, 2019
@longfin longfin added this to the 0.6.0 milestone Aug 22, 2019
@longfin longfin dismissed stale reviews from moreal and earlbread via c111e56 August 23, 2019 05:33
@longfin
Copy link
Member Author

longfin commented Aug 23, 2019

I've rebased to append it to 0.6.0.

earlbread
earlbread previously approved these changes Aug 23, 2019
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Store/IStore.cs Show resolved Hide resolved
@@ -53,6 +53,26 @@ public interface IStore

bool DeleteIndex(string @namespace, HashDigest<SHA256> hash);

/// <summary>
/// Forks block indexes from
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the term “block indexes” mean a sequence (chain) of block hashes and its index numbers? Or does it include state references? If it's latter the plural term seems natural for me as well, but if it's former I believe the singular term (i.e., “block index”) would listen more natural IMHO (as like the way other related methods are named, e.g., AppendIndex, DeleteIndex, IterateIndex).

Copy link
Member Author

@longfin longfin Aug 27, 2019

Choose a reason for hiding this comment

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

It's former. but I've confused about that because….

  • AppendIndex() and DeleteIndex() take only one argument.
  • Other iteration methods use plural form. (e.g. IterateBlockHashes(), IterateStateReferences())

So, how about keep ForkIndexes() in this PR and rename IterateIndex() to IterateIndexes() in other PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO in the case of this a singular noun “index” is more natural than a plural noun “indexes”/“indices,” in the same manner we say “a set of blocks” instead of “sets of blocks” to mean ISet[Block] in verbal language.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about this terminology once more, and it turns out the plural term has its own advantage, and I agree your suggestion to rename these method names to use the plural term “indexes” instead of the singular “index.” Some reasons:

  • Although relational databases tend to say “an index” for values in a column, we also say “indices” in books.
  • We already name a block's height by Block.Index, so it's natural to say a collection of Block.Indexes as the plural “indexes.”

- Use temporary memory to organize staterefs for avoid too many file writings
- IStore.StoreStateReferences() became to take blockHash and blockIndex instead of the block itself.
- Remove ReceivedAddress from StateReferenceDownloadState
- Add ForkBlockIndexes to IStore and LiteDBStore for fast fork
@longfin longfin merged commit aa5e357 into planetarium:master Aug 27, 2019
limebell pushed a commit to limebell/libplanet that referenced this pull request Jul 7, 2021
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