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

Fix: Batch process spent addresses to avoid out of memory issues #1314

Merged
merged 7 commits into from Jan 31, 2019

Conversation

Projects
None yet
4 participants
@GalRogozinski
Copy link
Member

GalRogozinski commented Jan 30, 2019

Description

When calculating spent addresses we may load too many addresses to memory and thus cause out of memory exceptions. To avoid that we process in batches and clear addresses we don't need.

Fixes #1273

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How Has This Been Tested?

@DyrellC will fill this in a comment

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

DyrellC and others added some commits Jan 29, 2019

@GalRogozinski GalRogozinski requested review from alon-e and DyrellC Jan 30, 2019

@alon-e

alon-e approved these changes Jan 30, 2019

Show resolved Hide resolved .../com/iota/iri/service/spentaddresses/impl/SpentAddressesServiceImpl.java Outdated
}

//If address is either null or equal to the coordinator address
if (addressHash.equals(Hash.NULL_HASH) ||

This comment has been minimized.

@alon-e

alon-e Jan 30, 2019

Member

instead of adding 2 more checks here, 999 & COO can be added to checkedAddress initially.

it would make the code more readable (less pre checks) and reduce to equal ops.

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 30, 2019

Author Member

In order to nicely implement that suggestion I will need to solve #1315 (so the Set will be always from hashes). I can also create construct the Hash using the factory but let's do it right already

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 30, 2019

Author Member

In the end #1315 is blocked so I used HashFactory

}

if (checkedAddresses != null) {
checkedAddresses.add(addressHash);

This comment has been minimized.

@alon-e

alon-e Jan 30, 2019

Member

you do checkedAddresses.add() before in an unsafe way (L164, even though it's in a try), is this check actually necessary?

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 30, 2019

Author Member

Missed it in the review.
It is necessary.

However, if I implement your above comment than we can be sure that checkedAddress won't be null and thus no null check will be neccessary

alon-e and others added some commits Jan 30, 2019

Fix number
Co-Authored-By: GalRogozinski <galrogogit@gmail.com>
@DyrellC
Copy link
Contributor

DyrellC left a comment

I've tested it on a few nodes now and there is definitely a reduction in cases where OOM errors occur. I like the addition of the getInitialUnspentAddresses call. Definitely simplifies the process and stops redundant checks in the loops.

/**
*
* Implementation of <tt>SpentAddressesService</tt> that calculates and checks spent addresses using the {@link Tangle}
*
*/
public class SpentAddressesServiceImpl implements SpentAddressesService {
private static final Logger log = LoggerFactory.getLogger(SpentAddressesServiceImpl.class);

This comment has been minimized.

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 31, 2019

Author Member

Even though we currently don't log
It is fine to have it in place

}

//Processing in batches in order to avoid OOM errors
private void processInBatches(int fromMilestoneIndex, int toMilestoneIndex, Collection<Hash> addressesToCheck,

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 31, 2019

Author Member

complexity 10 is marginal
this is fine

@alon-e

alon-e approved these changes Jan 31, 2019

@GalRogozinski

This comment has been minimized.

Copy link
Member Author

GalRogozinski commented Jan 31, 2019

Unit tests for this specific change are hard
General unit tests for this class are needed but this can be done in a separate PR

@GalRogozinski GalRogozinski merged commit 0c9ed77 into iotaledger:dev Jan 31, 2019

5 of 6 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
buildkite/iri-build-jar-prs Build #395 passed (8 minutes, 46 seconds)
Details
buildkite/iri-build-jar-prs/95735a37-93f8-46b3-a2b4-06d354e76e60 Passed (6 minutes, 17 seconds)
Details
buildkite/iri-build-jar-prs/build-iri-oracle8 Passed (2 minutes, 2 seconds)
Details
buildkite/iri-build-jar-prs/pull-from-repo Passed (17 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