-
Notifications
You must be signed in to change notification settings - Fork 222
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
improvement: don't cache all data of header map in memory during IBD #2147
Merged
bors
merged 1 commit into
nervosnetwork:develop
from
yangby-cryptape:pr/reduce-header-map
Jul 22, 2020
Merged
improvement: don't cache all data of header map in memory during IBD #2147
bors
merged 1 commit into
nervosnetwork:develop
from
yangby-cryptape:pr/reduce-header-map
Jul 22, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benchmark |
Benchmark Result
|
driftluo
previously approved these changes
Jul 2, 2020
It's better to default to the CKB data directory. Also expose the default in the generated config file. |
63e1cc8
to
d4c0ba5
Compare
benchmark |
@quake I re-push this PR and update the description, I try several strategies and compare the statistics, this is the easiest strategy, my original strategy is too complex but only a little benefit. |
Benchmark Result
|
d4c0ba5
to
c163d6c
Compare
quake
requested changes
Jul 8, 2020
92bad00
to
0ad104b
Compare
quake
previously approved these changes
Jul 9, 2020
driftluo
previously approved these changes
Jul 9, 2020
@yangby-cryptape please resolve the conflicts |
0ad104b
to
ef354fd
Compare
CKB - Pull Requests
automation
moved this from ✅ Reviewer approved
to 👀 Awaiting review
Jul 22, 2020
driftluo
approved these changes
Jul 22, 2020
quake
approved these changes
Jul 22, 2020
CKB - Pull Requests
automation
moved this from 👀 Awaiting review
to ✅ Reviewer approved
Jul 22, 2020
bors r=quake,driftluo |
Build succeeded:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Open a temporary rocksdb (default is
${ckb_data_dir}/tmp/ckb-tmp-*
) to store a part data of header map which exceeded the configured quota.Why not use the same rocksdb as block data?
It is just temporary data, all data was destroyed when ckb shutdown, so the format of data can be changed in any commits, we don't have to do migrations on it.
I don't want to create columns, it requires migrations. Also too many columns will affect performance.
If I use current existed columns, then traverse all data will be difficult.
At present, no data in chain database have key-prefix.
For example, if we use key
header-${header_hash}
to save headers, keyuncle-${uncle_hash}
to save uncles, keyblock-${block_hash}
to save blocks, then we can save them in a same column and we can traverse each of them use the key prefix (header-*
,uncle-*
,block-*
) easily.But we save almost all data just via
${hash}
, we can't know the type of data if we put them in a same column.So I can't use current existed columns without breaking the data traversing.
After IBD, there is about 450 MiB for current mainnet which height is about 2_200_000 with default rocksdb configuration (we can optimize the configuration later), but the size of header map is very small (in most of the time).
So we can just delete the entire rocksdb, we don't care about how to compress data and free space for rocksdb.
I use two thresholds to control the balance of memory and disk:
primary_limit
.primary_limit - backend_close_threshold
and there is no headers in rocksdb.I use a lot of
panic
, same as what we did inckb-store
. I trust the rocksdb and assume it always works well.I wrote two simple serialization and deserialization functions, because I think it is a private data structure, there is no need to make it too public (move to
ckb-types
) or bring heavy serialization and deserialization dependencies into this crate.Enable the
stats
feature to collect the statistics, the result of syncing mainnet from0
to2247420
is :