Bloom upgrade in beta #2609
Bloom upgrade in beta #2609
Conversation
NikVolf
added
A0-pleasereview 🤓
|
|
||
| fn check_bloom_exists(db: &Database) -> bool { | ||
|
|
||
| fn check_bloom_exists(db: &Arc<Database>) -> bool { |
There was a problem hiding this comment.
Justification for double pointer?
There was a problem hiding this comment.
i don't understand. you only need an &Database for get, no cloning required
| let hash_count_entry = db.get(DB_COL_ACCOUNT_BLOOM, ACCOUNT_BLOOM_HASHCOUNT_KEY) | ||
| .expect("Low-level database error"); | ||
|
|
||
| hash_count_entry.is_some() | ||
| } | ||
|
|
||
| fn check_space_match(db: &Arc<Database>) -> Result<(), usize> { |
There was a problem hiding this comment.
Justification for double pointer?
There was a problem hiding this comment.
ah right, no need there, updated
| let db_space = db.get(DB_COL_ACCOUNT_BLOOM, ACCOUNT_BLOOM_SPACE_KEY) | ||
| .expect("Low-level database error") | ||
| .map(|val| LittleEndian::read_u64(&val[..]) as usize) | ||
| .unwrap_or(1048576); |
There was a problem hiding this comment.
magic number should be extracted to constant.
There was a problem hiding this comment.
it is one-time magic number, it will never change
There was a problem hiding this comment.
Well, at least a comment would be helpful. I'm assuming this was the initial size of the bloom, but not 100% sure.
| } | ||
|
|
||
| println!("Adding accounts bloom (one-time upgrade)"); | ||
| let db = ::std::sync::Arc::new(source); | ||
| println!("Adding/expanding accounts bloom (one-time upgrade)"); |
There was a problem hiding this comment.
mix of printing and logging in this function. logging should be favored over printing wherever possible
There was a problem hiding this comment.
all migration messages are printed
There was a problem hiding this comment.
There should be no println!. Please use info!
There was a problem hiding this comment.
Should be fixed for other migration messages
|
users worried about migration that included full database copy, this one should be rather fast, no need for progress about 1 second for 500k accounts, might be several seconds for current amount |
|
@NikVolf is that on an HDD or SSD? The database might be up to 10M accounts by now. |
|
@rphmeier SSD |
|
@NikVolf, adding a progress would be a two-line change, |
|
@rphmeier added progress |
rphmeier
added
A8-looksgood 🦄
arkpar
added
A5-grumble 🔥
|
It took a few minutes on my machine for the first progress dot to appear |
|
does it make the database incompatible with current master or previous beta release? |
| @@ -138,13 +139,11 @@ impl StateDB { | |||
| } | |||
|
|
|||
| pub fn check_account_bloom(&self, address: &Address) -> bool { | |||
| trace!(target: "account_bloom", "Check account bloom: {:?}", address); | |||
There was a problem hiding this comment.
i wanted to rename to different target so that this messages don't mess with the bloom diag messages
is there any use of it? it is just endless spam if it is on
|
@arkpar |
arkpar
added
A7-looksgoodcantmerge 🙄
| LittleEndian::write_u64(&mut key, idx); | ||
| try!(batch.put(DB_COL_ACCOUNT_BLOOM, &key, &[0u8; 8])); | ||
|
|
||
| if idx % 10000 == 1 { progress.tick(); }; |
There was a problem hiding this comment.
tick already only prints something once every 100,000 times.
arkpar
added
A8-looksgood 🦄
Done so far: * created the dir crate in util * moved code from `ethstore/src/dir/paths.rs` to dir crate * rename `dir` module in ethstore to `accounts_dir` to distinguish it from the dir crate Is there a need to mutualize some code in the newly created `util/dir/src/lib.rs` ? If so, I would need some guidance as of which functions are to be mutualized.
Done so far: * created the dir crate in util * moved code from `ethstore/src/dir/paths.rs` to dir crate * rename `dir` module in ethstore to `accounts_dir` to distinguish it from the dir crate Is there a need to mutualize some code in the newly created `util/dir/src/lib.rs` ? If so, I would need some guidance as of which functions are to be mutualized.
No description provided.