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

store both mmr index and block height into database for output #2903

Merged
merged 5 commits into from
Aug 29, 2019
Merged

store both mmr index and block height into database for output #2903

merged 5 commits into from
Aug 29, 2019

Conversation

garyyu
Copy link
Contributor

@garyyu garyyu commented Jun 19, 2019

PR for #2889.

5~10 times speed improvement for /v1/chain/outputs and /v1/txhashset/outputs APIs, which will make the wallet restore, check and refresh outputs super fast comparing to current version.


For the database migration speed, I tested on current Floonet and spent 5 seconds for this migration on my laptop (Intel(R) Core(TM) i5-8210Y CPU @ 1.60GHz).

20190618 22:50:44.688 DEBUG grin_chain::chain - rebuild_height_for_pos: rebuilding 175910 output_pos's height...
20190618 22:50:49.731 DEBUG grin_chain::chain - rebuild_height_for_pos: done

The test examples of these APIs:

$ time curl -0 -XGET http://127.0.0.1:13413/v1/chain/outputs/byids?id=081f8c2343739bf28de457d80cc87efe62255a0bcc21e13022eefa7e5979b94529,080322b5046fce3390884b9035839ca296cdf7cfefbb4e7847dc226b35c0b6665f,083da72de4f6f22abce3c9ca9d826167c717b10eb44cfe80da4f715615738a7b85,094edc18bdaf8d0b0848b1d3e4991cd0e13a9d5aa755bc757469b4269abe4f6d5d,080384cb6acd92c1c15d20e9cd57fc74de70214866546b6dc3c9ff67d8ffcf374c
[
  {
    "commit": "081f8c2343739bf28de457d80cc87efe62255a0bcc21e13022eefa7e5979b94529",
    "height": 160001,
    "mmr_index": 470092
  },
  {
    "commit": "080322b5046fce3390884b9035839ca296cdf7cfefbb4e7847dc226b35c0b6665f",
    "height": 160001,
    "mmr_index": 470093
  },
  {
    "commit": "083da72de4f6f22abce3c9ca9d826167c717b10eb44cfe80da4f715615738a7b85",
    "height": 160003,
    "mmr_index": 470096
  },
  {
    "commit": "094edc18bdaf8d0b0848b1d3e4991cd0e13a9d5aa755bc757469b4269abe4f6d5d",
    "height": 160003,
    "mmr_index": 470097
  },
  {
    "commit": "080384cb6acd92c1c15d20e9cd57fc74de70214866546b6dc3c9ff67d8ffcf374c",
    "height": 160005,
    "mmr_index": 470099
  }
]
real	0m0.027s
user	0m0.010s
sys	0m0.009s


$ time curl -0 -XGET http://127.0.0.1:13413/v1/txhashset/outputs?start_index=100000\&max=1000 2>/dev/null >/dev/null

real	0m0.080s
user	0m0.009s
sys	0m0.009s

@garyyu garyyu added this to the 2.x.x milestone Jun 19, 2019
@garyyu garyyu requested a review from antiochp June 19, 2019 04:04
chain/src/chain.rs Outdated Show resolved Hide resolved
@garyyu garyyu changed the base branch from master to milestone/2.x.x July 23, 2019 05:41
@quentinlesceller quentinlesceller changed the base branch from milestone/2.x.x to master July 24, 2019 15:42
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Yeah this is nice.
I'd like to do some local testing with this over the next couple of days before we merge but 👍.

@@ -396,7 +396,7 @@ where
{
fn deser_if_prefix_match(&self, key: &[u8], value: &[u8]) -> Option<(Vec<u8>, T)> {
let plen = self.prefix.len();
if plen == 0 || key[0..plen] == self.prefix[..] {
if plen == 0 || (key.len() >= plen && key[0..plen] == self.prefix[..]) {
Copy link
Member

Choose a reason for hiding this comment

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

What was this change needed for?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I get it now after rereading it. We just check the length for safety so it doesn't panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Actually I saw a panic here on a test, don't know exact reason, but not bad to give a safe protection here.

@antiochp
Copy link
Member

@garyyu Is this good to go? I can merge if so.

@garyyu
Copy link
Contributor Author

garyyu commented Aug 27, 2019

@antiochp ok if you don't need that new type for the (u64, u64). I have run a lot of tests on several nodes for this PR, no problem found.

@antiochp
Copy link
Member

Testing this locally.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

In rebuild_index we do the following -

		for pos in self.output_pmmr.leaf_pos_iter() {
			if let Some(out) = self.output_pmmr.get_data(pos) {
				self.batch.save_output_pos(&out.commit, pos)?;
				count += 1;
			}
		}

i.e. We are still calling save_output_pos() and I think we should be calling the new save_output_pos_height() instead?

@garyyu
Copy link
Contributor Author

garyyu commented Aug 29, 2019

In rebuild_index we do the following -

		for pos in self.output_pmmr.leaf_pos_iter() {
			if let Some(out) = self.output_pmmr.get_data(pos) {
				self.batch.save_output_pos(&out.commit, pos)?;
				count += 1;
			}
		}

i.e. We are still calling save_output_pos() and I think we should be calling the new save_output_pos_height() instead?

No, I keep it to avoid a big refactoring. Instead, after each time calling this rebuild_index, there will be another rebuild_height_for_pos calling to migrate pos index to pos+height index.

There're 2 locations for this rebuild_index:


(BTW, I want another PR to give a complete refactoring on the current txhashset_write function, which currently has multiple rebuild split into multiple locking: rebuild_header_mmr, rebuild_index, rebuild_header_mmr, rebuild_height_for_pos. There could be a race condition here if a new block coming in and seize the locking among these rebuild, even in a very low possibility.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants