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

Add serialization and blockstore stats for common tests #33

Closed
wants to merge 10 commits into from

Conversation

austinabell
Copy link

  • Adds serialization vectors and blockstore stats to the tests that are replicated

The stats include reads, writes, bytes read, bytes written to use to compare against since Filecoin uses reads, writes, and bytes written for gas usage.

Rust equivalent test additions: ChainSafe/forest#716

There are a few cases where there are many more writes than necessary, but I will open up an issue to explain

@arajasek arajasek added this to In Review in Lotus+Actors Board via automation May 3, 2021
@arajasek arajasek self-requested a review May 3, 2021 17:07
@jennijuju jennijuju added the P3 label May 17, 2021
}

assert.Equal(t, "bafy2bzaced44wtasbcukqjqicvxzcyn5up6sorr5khzbdkl6zjeo736f377ew", c.String())
assert.Equal(t, bsStats{r: 12, w: 12, br: 573, bw: 573}, trackingBs.stats)
Copy link
Member

Choose a reason for hiding this comment

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

these stats 😱 are why I think we need to work on a collapsable form #17 (comment) .. I don't have any idea how common this kind of perverse sparseness is in actual Filecoin data though

Copy link
Author

Choose a reason for hiding this comment

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

the only stat that isn't used in Filecoin was bytes read, all other 3 are relevant

@rvagg
Copy link
Member

rvagg commented Jul 12, 2021

these lgtm but needs a rebase and to have the commented code dealt with - can TestMain come back? and TestRoundTrip I think has been replaced

There are a few cases where there are many more writes than necessary, but I will open up an issue to explain

would be very interested to hear more on this if you still have these top of mind @austinabell

@austinabell
Copy link
Author

austinabell commented Jul 12, 2021

these lgtm but needs a rebase and to have the commented code dealt with - can TestMain come back? and TestRoundTrip I think has been replaced

There are a few cases where there are many more writes than necessary, but I will open up an issue to explain

would be very interested to hear more on this if you still have these top of mind @austinabell

Sorry, I don't have the bandwidth to do this anytime soon, the motivation was to make sure blockstore usages and roots of the amt is consistent for all cases as there were some strange patterns, but I don't know the current status or if this would be useful for other impls now that this is probably stabilized. Would be good to make sure things don't change in any case, but I don't have an opinion or motivation to push for this myself so feel free to close or modify this PR.

@austinabell
Copy link
Author

closing because stale

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

Successfully merging this pull request may close these issues.

None yet

3 participants