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

Perf: Massively improved the performance of the replayMilestones method #1197

Conversation

hmoog
Copy link

@hmoog hmoog commented Nov 26, 2018

Description

This PR refactors the replayMilestones method to first accumulate all changes and then apply them in a single call. This allows us to modify the milestone without having to create a copy of the state first. Since the Snapshot and its corresponding SnapshotState are huge datastructures, this change reduces the time required for a rebuild of the ledger state after a IRI restart from a few hours to a few seconds (for a non-snapshotted database).

Type of change

  • Performance refactor (a non-breaking change)

Checklist:

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
@iotaledger iotaledger deleted a comment Nov 28, 2018
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Awesome!

Objects.equals(metaData, ((SnapshotImpl) obj).metaData) &&
Objects.equals(skippedMilestones, ((SnapshotImpl) obj).skippedMilestones);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It will also be nice to have a toString() here.
The toString() may only print SnapshotMetadata` toString() to not make it a reasonable size (index, hash, hasSolidEntryPoints). You can also decide to have it print the state, but it will be long.

Whatever you think will help with debugging

Copy link
Author

Choose a reason for hiding this comment

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

We can add that in another PR - maybe create an issue to check if all models have these toString methods.

Copy link
Contributor

@alon-e alon-e left a comment

Choose a reason for hiding this comment

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

great PR and clever idea!

@@ -112,6 +113,10 @@ public void applyStateDiff(SnapshotStateDiff diff) throws SnapshotException {
if (balances.computeIfPresent(addressHash, (hash, aLong) -> balance + aLong) == null) {
balances.putIfAbsent(addressHash, balance);
}

if (balances.get(addressHash) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@GalRogozinski GalRogozinski merged commit 6ee68bc into iotaledger:dev-localsnapshots Nov 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants