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

feat: remove orphans #646

Merged
merged 7 commits into from
Jan 23, 2023
Merged

feat: remove orphans #646

merged 7 commits into from
Jan 23, 2023

Conversation

cool-develope
Copy link
Collaborator

ref: #592, cosmos/cosmos-sdk#12989

Context

We assume the intermediate versions are unnecessary and by keeping the versions in sequence, we don't need to load all versions and we can keep only the fristVersion and latestVersion. It will resolve #637 .

What does this PR do?

  • Remove DeleteVersion, DeleteVesions, and DeleteVersionsRange
  • Add a new API of DeleteVersionsTo
  • Remove orphans from the storage
  • Remove LazyLoadVersion

@cool-develope cool-develope requested a review from a team as a code owner December 8, 2022 20:09
mutable_tree.go Fixed Show fixed Hide fixed
defer itr.Close()

nversion := int64(0)
for ; itr.Valid(); itr.Next() {

Check warning

Code scanning / CodeQL

Unreachable statement

This statement is unreachable.
@cool-develope
Copy link
Collaborator Author

@yihuang , it is overlapped with your PR #641, I am open to your opinions. Please review it.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 8, 2022

This pull request introduces 1 alert when merging 946c32d into 992120d - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@yihuang
Copy link
Collaborator

yihuang commented Dec 9, 2022

We assume the intermediate versions are unnecessary and by keeping the versions in sequence

There's a use case to keep every nth version as snapshots to rebuild any version on the fly by replaying the change set from versiondb or file streamer outputs. I think it should be easy to support the current behaviour.

I believe your prune(diff) algorithm is equavalent to the one in #641, adding some small adjustments.
Currently the tree diff implementation looks something like this:

def find_orphaned_nodes(v1, v2):
    pass

def delete_version(v):
    for orphaned_node in find_orphaned_nodes(v, v+1):
        delete_node(orphaned_node)

Do several adjustments:

  • find successor version dynamically
  • find predecessor version, and don't delete nodes beyond predecessor version.
    Then it'll work perfectly with non-continuous versions.
def delete_version(v):
    predecessor = find_predecessor(v)  # default to 0 if no predecessor
    successor = find_successor(v) # if there's no successor, we should return error
    for orphaned_node in find_orphaned_nodes(v, successor):
        if orphaned_node.version <= predecessor:
            # this node must be referenced by the predecessor version, so don't delete
        else:
            delete_node(orphaned_node)

The orphaned_node.version <= predecessor check can be further embed into the tree traversals itself to make it more performant.

The diff algorithm is just an implementation detail, the main issue is why drop the support for the gap versions, I think it's a useful feature. I believe we should at least discuss these two issues(remove orphan and support gap versions, maybe the slow startup issue as well) separately, they can be solved separately.

@yihuang
Copy link
Collaborator

yihuang commented Dec 9, 2022

It will resolve #637 .

I think startup time issue alone can be solved by lazy load mode.

iterator.go Outdated
return
}
node := iter.GetNode()
iter.nodesToVisit = iter.nodesToVisit[:len(iter.nodesToVisit)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask is this rm last node to allow visit left node if skipped (when left and right node exist in last iteration)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is to pop the visited node from the stack?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, it simulates pre-order DFS, so it assumes the root of subtree is already visited and pops it from the stack.

@tac0turtle
Copy link
Member

tac0turtle commented Dec 9, 2022

I would argue we should reduce the feature set and off load things like old commitments outside the node. Something like commitment streaming would solve this issue and would reduce the complexity in the code we maintain.

Secondly the feature in the sdk of intermittent states was removed a while ago and I don't think anyone has come asking for it back outside of crypto.com I believe.

Thirdly, there is a long term objective of trying experiment with different treee algorithms. The more we dice the feature set of iavl the easier it will be to swap trees for research in the future. Also this feature wasn't present on adr40 so surprised we need to keep it here.

@yihuang
Copy link
Collaborator

yihuang commented Dec 9, 2022

I would argue we should reduce the feature set and off load things like old commitments outside the node. Something like commitment streaming would solve this issue and would reduce the complexity in the code we maintain.

Secondly the feature in the sdk of intermittent states was removed a while ago and I don't think anyone has come asking for it back outside of crypto.com I believe.

Thirdly, there is a long term objective of trying experiment with different treee algorithms. The more we dice the feature set of iavl the easier it will be to swap trees for research in the future. Also this feature wasn't present on adr40 so surprised we need to keep it here.

I understand the desire to simplify things here, it's just I don't see supporting this feature make other things too complicated. But yeah, maybe in the long run, people do need to have alternative implementations, as long as we keep the root hashes compatible.

@cool-develope
Copy link
Collaborator Author

cool-develope commented Dec 9, 2022

tbh, I don't like the word lazy loading, I think we should keep the primary functionalities as small as possible and can provide more full extra functionalities, the typical case of extra functionality is import/export.
for example, we can solve the version gap issue by arguing import/export. we can add fromVersion parameter to export, so that it will export only new nodes which are updated after fromVersion. In this way, we can separate the storage as operating storage and snapshots storage.

}
pNode := prevIter.GetNode()

if orgNode != nil && bytes.Equal(pNode.hash, orgNode.hash) {
Copy link
Collaborator

@yihuang yihuang Dec 12, 2022

Choose a reason for hiding this comment

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

Are you sure preorder traversal is correct for this, I'm still trying to reason about the correctness of this algorithm, it seems we need an in-order traversal here which is ordered by node keys.
What's important here is curIter and prevIter must visit the sequence of orgNodes in the same order, right?
Or we can keep the set of orgNode in memory, so we can traverse the trees in whatever order.

Copy link
Collaborator

@yihuang yihuang Dec 12, 2022

Choose a reason for hiding this comment

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

But we do need preorder to try to skip the subtree early on, so we need to check twice for a branch node both preorder and inorder.
For example, if the branch node match the current orgNode, we want to check with preorder to avoid traverse into the subtree.
But if the branch node's left children will match the current orgNode, the branch node need to be checked again against the new orgNode, because it could be a match too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pre-order or in-order are not important here, right? Because we only care about the order of leaves. I don't like keep orgNodes or orphans in memory, those amounts may be enormous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, orgNode could be branch nodes

@yihuang
Copy link
Collaborator

yihuang commented Dec 12, 2022

It seems always deleting versions from the beginning is not optimal, because in that case we always do a full diff without a predecessor to limit the traversal space, just an intuition though, need closer analysis.

@cool-develope
Copy link
Collaborator Author

It seems always deleting versions from the beginning is not optimal, because in that case we always do a full diff without a predecessor to limit the traversal space, just an intuition though, need closer analysis.

yeah, you are right. It is designed before iterator implementation, I will re-struct it

@cool-develope
Copy link
Collaborator Author

It seems always deleting versions from the beginning is not optimal, because in that case we always do a full diff without a predecessor to limit the traversal space, just an intuition though, need closer analysis.

yeah, you are right. It is designed before iterator implementation, I will re-struct it

It seems like there is no way for now, maybe there will be a way using new node key (version + path)

@yihuang
Copy link
Collaborator

yihuang commented Dec 19, 2022

With new node key format, we can do the diff with two simple db iterations, and:

  • if the nonce is assigned ordered by node.key, we can do two iterations simultaneously with constant memory.
  • otherwise, we can do two iterations separately, keeping the set shared node keys in temporary memory.

@tac0turtle
Copy link
Member

@cool-develope should we review and merge this before #650

@cool-develope
Copy link
Collaborator Author

cool-develope commented Jan 3, 2023

@cool-develope should we review and merge this before #650

no, #650 entirely belongs its implementation.
I want to close this PR when merging #650 for review purposes or we can merge this first.

@yihuang
Copy link
Collaborator

yihuang commented Jan 4, 2023

@cool-develope can you address the traversal order issue, I used the temporary map approach to extract state changes here but following your diff algorithm in general.

@yihuang
Copy link
Collaborator

yihuang commented Jan 10, 2023

For example, the current node (say N1) in prevIter don't match the current orgNode, but one node in it's left branch matched it and cause orgNode to bump to N1, but it's won't be checked anymore, because the N1 in prevIter is already checked.

No, if N1 is not matched, then we will keep traverse within N1's subtree. The skip is applied only if we meet the same node with orgNode.

Say N1 don't match current orgNode O1, but N1's left child match O1, is it possible that a future orgNode will be N1? In that case, N1 is already checked, won't match again. I hope it's not possible, but we need to prove it.

@cool-develope
Copy link
Collaborator Author

Say N1 don't match current orgNode O1, but N1's left child match O1, is it possible that a future orgNode will be N1? In that case, N1 is already checked, won't match again. I hope it's not possible, but we need to prove it.

no, it is not possible, there is no overlapping between orgNodes, just think as a group of subtree. the orgNodes should be a group of subtree, right?

@yihuang
Copy link
Collaborator

yihuang commented Jan 10, 2023

Say N1 don't match current orgNode O1, but N1's left child match O1, is it possible that a future orgNode will be N1? In that case, N1 is already checked, won't match again. I hope it's not possible, but we need to prove it.

no, it is not possible, there is no overlapping between orgNodes, just think as a group of subtree. the orgNodes should be a group of subtree, right?

Yeah, I guess that make sense.

@yihuang
Copy link
Collaborator

yihuang commented Jan 11, 2023

I guess the reasoning goes like this:

  • The sequence of orgNode is a list of root nodes of disjoint sub-trees, because when traversing curIter, the sub-tree is skipped when root node is found.
  • A sequence of root nodes of disjoint sub-trees are visited in the same order in different versions with pre-order traversal, is there a more formal argument for this assumption?

}
pNode := prevIter.GetNode()

if orgNode != nil && bytes.Equal(pNode.hash, orgNode.hash) {
Copy link
Collaborator

@yihuang yihuang Jan 11, 2023

Choose a reason for hiding this comment

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

is it more efficient to do an extra pointer comparison here: (pNode == orgNode || bytes.Equal(pNode.hash, orgNode.hash))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good point, if they are from cache

yihuang added a commit to yihuang/iavl that referenced this pull request Jan 11, 2023
yihuang added a commit to yihuang/iavl that referenced this pull request Jan 11, 2023
@cool-develope
Copy link
Collaborator Author

I guess the reasoning goes like this:

  • The sequence of orgNode is a list of root nodes of disjoint sub-trees, because when traversing curIter, the sub-tree is skipped when root node is found.
  • A sequence of root nodes of disjoint sub-trees are visited in the same order in different versions with pre-order traversal, is there a more formal argument for this assumption?

no, looks good, say again the order method is not important here

@yihuang
Copy link
Collaborator

yihuang commented Jan 11, 2023

I guess the reasoning goes like this:

  • The sequence of orgNode is a list of root nodes of disjoint sub-trees, because when traversing curIter, the sub-tree is skipped when root node is found.
  • A sequence of root nodes of disjoint sub-trees are visited in the same order in different versions with pre-order traversal, is there a more formal argument for this assumption?

no, looks good, say again the order method is not important here

order is important here, if the sequence of orgNode are visited in different order in two versions, that's definitely problematic, although that seems not to be the case here.

@cool-develope
Copy link
Collaborator Author

order is important here, if the sequence of orgNode are visited in different order in two versions, that's definitely problematic, although that seems not to be the case here.

sorry, you are right, I mean the traversal order is not important, for example in-order traverse of prevIter will also work

@yihuang
Copy link
Collaborator

yihuang commented Jan 11, 2023

order is important here, if the sequence of orgNode are visited in different order in two versions, that's definitely problematic, although that seems not to be the case here.

sorry, you are right, I mean the traversal order is not important, for example in-order traverse of prevIter will also work

yeah, agree, as long as they visit the orgNodes in the same order.

nodedb.go Show resolved Hide resolved
nodedb.go Outdated Show resolved Hide resolved
@kocubinski
Copy link
Member

Seems generally OK but I think we should have some tests for DeleteVersionsTo and traverseOrphans. I'm still proving to myself that that algorithm works as expected. Thorough coverage of edge cases would help.

iterator.go Outdated Show resolved Hide resolved
Copy link
Member

@kocubinski kocubinski left a comment

Choose a reason for hiding this comment

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

Tested, audited, LGTM. I still think we should have some basic tests of traverseOprhans though. Roughly what I did in my branch but more formal, I just compared STDOUT to my expectations. I can commit some of these later this week if you want.

It will probably also be useful to run some benchmark tests on larger data sets validating inputs/outputs at a high level. More like fuzz testing.

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

I'm not sure about dropping the support for deleting versions in middle, but other than that, LGTM.

@cool-develope cool-develope merged commit bc94180 into master Jan 23, 2023
@cool-develope cool-develope deleted the 592/remove_orphans branch January 23, 2023 17:24
yihuang added a commit to yihuang/iavl that referenced this pull request Jan 26, 2023
fix test

rename

try to lower memory usage

fix lint
tac0turtle added a commit that referenced this pull request Jan 27, 2023
mergify bot pushed a commit that referenced this pull request Jan 27, 2023
tac0turtle pushed a commit that referenced this pull request Jan 27, 2023
ankurdotb pushed a commit to cheqd/iavl that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iavl start up time is slow
7 participants