Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Blocks and snapshot compression #1687

Merged
merged 118 commits into from
Jul 27, 2016
Merged

Blocks and snapshot compression #1687

merged 118 commits into from
Jul 27, 2016

Conversation

keorn
Copy link

@keorn keorn commented Jul 21, 2016

  • reduce Compressible interface
  • check migrations
  • benchmark effect of blocks DB compression on syncing

@keorn keorn removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jul 22, 2016
let again_raw = UntrustedRlp::new(&compact_vec).decompress(RlpType::Snapshot);
assert_eq!(raw, again_raw.to_vec());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

replace spaces with tabs

// Shortest decompressed account is 70, so simply try to swap the value.
Ok(ref p) if p.value_len < 70 => simple_swap(),
_ => {
if let Ok(d) = rlp.data() {
Copy link
Collaborator

@debris debris Jul 23, 2016

Choose a reason for hiding this comment

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

line 109 already proves that rlp is data. You can use expect(...) here or replace line 109 with:

if let Ok(d) = rlp.data() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

*edit, my previous comment may not be valid. is_data does not check validity of data, whereas rlp.data() does. Is rlp expected to be invalid here?

Copy link
Author

Choose a reason for hiding this comment

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

It can be invalid, since data field is not always an RLP.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.03%) to 75.429% when pulling 5b39dab on blockscompression into aafb014 on master.

@debris
Copy link
Collaborator

debris commented Jul 23, 2016

overall LGTM

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.04%) to 75.41% when pulling cede71e on blockscompression into aafb014 on master.

@gavofyork
Copy link
Contributor

i guess it's not yet optional?

@keorn
Copy link
Author

keorn commented Jul 23, 2016

Ah ok, will make it optional for blocks.

@keorn keorn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 23, 2016
@gavofyork
Copy link
Contributor

ok looks good. actually results in a ~ 8% speedup for the first 200k blocks.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 27, 2016
@keorn
Copy link
Author

keorn commented Jul 27, 2016

I got some speedup on full sync too.
Master:

real    405m49.204s
user    587m33.366s
sys     27m45.241s

With blocks compression:

real    395m6.620s
user    557m51.039s
sys     25m58.350s

@keorn keorn added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 27, 2016
Conflicts:
	ethcore/src/blockchain/blockchain.rs
@keorn keorn added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Jul 27, 2016
@keorn keorn closed this Jul 27, 2016
@keorn keorn reopened this Jul 27, 2016
@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-0.3%) to 86.904% when pulling c220631 on blockscompression into c65ee93 on master.

@gavofyork gavofyork merged commit 02cf486 into master Jul 27, 2016
@gavofyork gavofyork deleted the blockscompression branch July 27, 2016 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants