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

Increase rescan performance of old milestones after IRI restart #1204

Merged
merged 9 commits into from Dec 10, 2018

Conversation

Projects
None yet
4 participants
@hmoog
Copy link
Contributor

hmoog commented Nov 29, 2018

Description

IRI always scans all potential milestone candidates after a restart. Depending on the database size this can take a really long time (hours in the worst case). If we have a local snapshot we can always immediately discard the transactions prior to the snapshot point because the node doesn't need them anymore. This increases the restart performance of IRI massively (few seconds).

Type of change

  • Enhancement (a non-breaking change which adds functionality)

Checklist:

Please delete items that are not relevant.

  • 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 from codacy-bot Dec 1, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Dec 1, 2018

@GalRogozinski GalRogozinski requested review from alon-e and GalRogozinski Dec 2, 2018

@alon-e
Copy link
Member

alon-e left a comment

I think validateMilestone should be stateless.
it's worse enough that the depth/index is capped by milestoneIndex >= 0x200000 which is only relevant to mainnet but not to other networks w/ theoretically more leaves (+ #1180)

so adding IRRELAVENT inside the function makes it, even more, dependant on state - think of test where the hardcoded testing milestone is below the starting milestone.

-> my proposal is to pull this out of the validate milestone and into a function that checks if the index is valid or needed before calling the validation.

@hmoog

This comment has been minimized.

Copy link
Contributor

hmoog commented Dec 6, 2018

i changed the check to be part of the milestone tracker instead

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

just another little change

@@ -160,10 +160,9 @@ public void resetCorruptedMilestone(int index) throws MilestoneException {
}

@Override
public MilestoneValidity validateMilestone(TransactionViewModel transactionViewModel, SpongeFactory.Mode mode,
int securityLevel) throws MilestoneException {
public MilestoneValidity validateMilestone(TransactionViewModel transactionViewModel, int milestoneIndex,

This comment has been minimized.

@GalRogozinski

GalRogozinski Dec 6, 2018

Member

nice addition of parameter

@@ -209,8 +208,13 @@ public MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transact
transaction.getCurrentIndex() == 0) {

int milestoneIndex = milestoneService.getMilestoneIndex(transaction);
if (milestoneIndex <= snapshotProvider.getInitialSnapshot().getIndex()) {
return INVALID;
}

This comment has been minimized.

@GalRogozinski

GalRogozinski Dec 6, 2018

Member

I think Alon's beef was that the validateMilestone depended on the milestone Index. On which he was correct, we don't want other potential callers of this method to be blocked by the index.

He didn't say anything about your IRRELEVANT enum. I think that returning an INVALID on a vaild milestone will be very misleading to anyone in the future (it is at least misleading for me). I would bring back this enum value.

This comment has been minimized.

@GalRogozinski

GalRogozinski Dec 6, 2018

Member

Actually I have a better idea... Instead of adding another enum to validity (IRRELEVANT) (which doesn't make much sense), analyzeMilestoneCandidate can return a boolean: true if successfully analyzed, false otherwise.

If the milestoneIndex is below the snapshot then return true and add a little comment: "has already been analyzed if it is below the snapshot" (feel free to change the wording).

I think this makes the most sense.

This comment has been minimized.

@alon-e

alon-e Dec 7, 2018

Member

I agree w/ @GalRogozinski that returning INVALID may not be the best answer. other than that lgtm.

This comment has been minimized.

@hmoog

hmoog Dec 7, 2018

Contributor

i changed it accordingly

hmoog added some commits Dec 7, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Dec 7, 2018

return true;
}

switch (milestoneService.validateMilestone(transaction, milestoneIndex, SpongeFactory.Mode.CURLP27, 1)) {

This comment has been minimized.

This comment has been minimized.

@GalRogozinski

GalRogozinski Dec 9, 2018

Member

Let's try not to ignore codacy.

Even thought the way you did it is less verbose, I think it is still clearer and more readable to have the default case inside the switch brackets. You can also add a little comment:
//We can consider the milestone candidate processed and move on w/o farther action

This comment has been minimized.

@hmoog

hmoog Dec 10, 2018

Contributor

i think this is really unnecessary here since the switch is used as a faster / easier to read version of an if then else if then but if you think it helps i can add an empty default with that comment .. :P

@alon-e

alon-e approved these changes Dec 7, 2018

@@ -203,14 +198,19 @@ public MilestoneValidity analyzeMilestoneCandidate(Hash transactionHash) throws
* {@link MilestoneSolidifier} that takes care of requesting the missing parts of the milestone bundle.<br />
*/
@Override
public MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transaction) throws MilestoneException {
public boolean processMilestoneCandidate(TransactionViewModel transaction) throws MilestoneException {

This comment has been minimized.

@GalRogozinski

GalRogozinski Dec 9, 2018

Member

👍 on the name change

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Great

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Great!

@GalRogozinski GalRogozinski merged commit 08444bc into iotaledger:dev-localsnapshots Dec 10, 2018

6 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
buildkite/iri-build-jar-prs Build #274 passed (9 minutes, 40 seconds)
Details
buildkite/iri-build-jar-prs/9d6ebb0f-8f7e-487b-a4ab-faf7354ad9d3 Passed (6 minutes, 29 seconds)
Details
buildkite/iri-build-jar-prs/build-iri-oracle8 Passed (2 minutes, 25 seconds)
Details
buildkite/iri-build-jar-prs/pull-from-repo Passed (30 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment