replace all trit representations from int[] to byte[] by alon-e · Pull Request #879 · iotaledger-archive/iri

alon-e · 2018-07-21T19:54:26Z

Description

Replace all trit representations from int[] to byte[]
Currently, Trits (-1,0,1) are stored in integer arrays, which waste memory (and compute to GC).
This PR replaces all usage trit representations from int[] to byte[].

Fixes #799

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

Fixed unit tests
Syncing a node with brach - improved memory and CPU footprint be > 50%

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

fix tests accordingly.

alon-e · 2018-07-21T20:04:07Z

src/test/java/com/iota/iri/controllers/TransactionViewModelTest.java

+            Hash hash = getRandomTransactionHash();
+            TransactionViewModel transactionViewModel = new TransactionViewModel(trits, hash);
+            transactionViewModel.store(tangle);
+            assertArrayEquals(transactionViewModel.trits(), TransactionViewModel.fromHash(tangle, transactionViewModel.getHash()).trits());


re-enabled test and fixed, could move to another PR if you want.

alon-e · 2018-07-21T20:04:17Z

src/test/java/com/iota/iri/controllers/TransactionViewModelTest.java

+            Hash hash = getRandomTransactionHash();
+            TransactionViewModel transactionViewModel = new TransactionViewModel(trits, hash);
+            transactionViewModel.store(tangle);
+            assertArrayEquals(transactionViewModel.getBytes(), TransactionViewModel.fromHash(tangle, transactionViewModel.getHash()).getBytes());


re-enabled test and fixed, could move to another PR if you want.

alon-e · 2018-07-21T20:05:59Z

src/test/java/com/iota/iri/controllers/TransactionViewModelTest.java

        Transaction transaction = new Transaction();

-        int[] trits = Arrays.stream(new int[TransactionViewModel.SIGNATURE_MESSAGE_FRAGMENT_TRINARY_SIZE]).map(i -> seed.nextInt(3)-1).toArray();
+        byte[] trits = new byte[TransactionViewModel.SIGNATURE_MESSAGE_FRAGMENT_TRINARY_SIZE];


getRandomTransaction impl. appears a few times, better to move to a Utils class.
but not part of this scope.

alon-e · 2018-07-21T20:06:20Z

src/test/java/com/iota/iri/controllers/TransactionViewModelTest.java

                TransactionViewModel.BRANCH_TRANSACTION_TRINARY_SIZE);
        return trits;
    }
-    public static int[] getRandomTransactionWithTrunkAndBranchValidBundle(Hash trunk, Hash branch) {


removed b/c not used.

GalRogozinski

I disliked the if hack to differentiate between the construction of bytes and trits.

If you want to save time I can agree on merging this as long as there is a github issue opened on this matter.

GalRogozinski · 2018-07-22T06:25:07Z

src/main/java/com/iota/iri/TransactionValidator.java

+        } else {
+            return validate(trits, minWeightMagnitude, SpongeFactory.create(SpongeFactory.Mode.CURLP81));
+        }
    }


Instead of this hack, perhaphs create two methods:
validateTrits and validateBytes.

The above change is just confusing (even though it works)...

GalRogozinski · 2018-07-22T06:29:25Z

src/main/java/com/iota/iri/controllers/TransactionViewModel.java

+            this.hash = hash;
+            weightMagnitude = this.hash.trailingZeros();
+            transaction.type = FILLED_SLOT;
+        }


I would create two static factory methods:
createFromBytes and createFromTrits and make the constructor private.

I'll open an issue on this. but it shouldn't block the goodness from this PR imo.

GalRogozinski · 2018-07-22T06:39:21Z

src/main/java/com/iota/iri/hash/ISSInPlace.java

@@ -16,7 +16,7 @@ public class ISSInPlace {
  private static final int MIN_TRIT_VALUE = -1, MAX_TRIT_VALUE = 1;
  private static final int MIN_TRYTE_VALUE = -13, MAX_TRYTE_VALUE = 13;


change from int to byte.
Your code works either way, but still...

GalRogozinski · 2018-07-22T06:56:56Z

src/main/java/com/iota/iri/model/Hash.java

+            System.arraycopy(source, sourceOffset, dest, 0, dest.length);
+            this.tritSafe = new TritSafe(dest);
+        }
    }


Again I would encourage the use of factory methods...

I'll open an issue on this. but it shouldn't block the goodness from this PR imo.

GalRogozinski · 2018-07-22T07:03:27Z

src/test/java/com/iota/iri/controllers/TransactionViewModelTest.java

        Transaction transaction = new Transaction();

-        int[] trits = Arrays.stream(new int[TransactionViewModel.SIGNATURE_MESSAGE_FRAGMENT_TRINARY_SIZE]).map(i -> seed.nextInt(3)-1).toArray();
+        byte[] trits = new byte[TransactionViewModel.SIGNATURE_MESSAGE_FRAGMENT_TRINARY_SIZE];


GalRogozinski · 2018-07-22T07:06:50Z

src/test/java/com/iota/iri/controllers/TransactionViewModelTest.java

+        }
+
+        return out;
    }


I agree that this is more reasdable than the one liner.
Another option was to not change the code and just add new lines before each aggregate call.

Anyhows, both variations are fine with me.

…tes in bytes.

GalRogozinski

Changes will be handled in a different issue

alon-e requested review from GalRogozinski and th0br0 July 21, 2018 19:54

replace all trit representations from int[] to byte[]

aa06b2e

fix tests accordingly.

alon-e force-pushed the feat/tritsAsBytes branch from b0612d1 to aa06b2e Compare July 21, 2018 20:01

alon-e commented Jul 21, 2018

View reviewed changes

make codacy changes

3cc210d

iotaledger-archive deleted a comment Jul 22, 2018

GalRogozinski reviewed Jul 22, 2018

View reviewed changes

make tx validate input type more explicit, MIN/MAX values of trit/try…

b9371a4

…tes in bytes.

GalRogozinski approved these changes Jul 22, 2018

View reviewed changes

GalRogozinski merged commit f2b2d04 into iotaledger-archive:dev Jul 22, 2018

alon-e mentioned this pull request Jul 22, 2018

use factories instead of conditional constructors in: TxVM & Hash #880

Open

		@@ -16,7 +16,7 @@ public class ISSInPlace {
		private static final int MIN_TRIT_VALUE = -1, MAX_TRIT_VALUE = 1;
		private static final int MIN_TRYTE_VALUE = -13, MAX_TRYTE_VALUE = 13;

Conversation

alon-e commented Jul 21, 2018

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GalRogozinski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants