perf: improve decoding in CallTransaction #2647

Vovchyk · 2024-07-26T12:32:29Z

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

fmacleal

Good job. I have added some comments, mostly with improvements suggestions to improve the readability of some tests. They doesn't prevent the approval, since it wouldn't change the current logic.

fmacleal · 2024-08-07T12:18:04Z

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java

+    @Test
+    void testBytesLength() {
+        assertEquals(0, Bytes.of(new byte[]{}).slice(0, 0).length());
+        assertEquals(0, Bytes.of(new byte[]{1}).slice(0, 0).length());
+        assertEquals(1, Bytes.of(new byte[]{1}).slice(0, 1).length());
+        assertEquals(0, Bytes.of(new byte[]{1,2,3}).slice(1, 1).length());
+        assertEquals(1, Bytes.of(new byte[]{1,2,3}).slice(1, 2).length());
+        assertEquals(2, Bytes.of(new byte[]{1,2,3}).slice(0, 2).length());
+        assertEquals(3, Bytes.of(new byte[]{1,2,3}).slice(0, 3).length());
+    }


suggestion:

Identify the scenarios you are testing it's a bit tricky, because we don't have clues with variable names and so on. I think we could get advantage of @ParameterizedTest and a String to tell us the scenario you are testing. Here is a snippet code in how you could change this test. I think that we could get benefit of it or use similar ideas for the other tests below.

Suggested change

@Test

void testBytesLength() {

assertEquals(0, Bytes.of(new byte[]{}).slice(0, 0).length());

assertEquals(0, Bytes.of(new byte[]{1}).slice(0, 0).length());

assertEquals(1, Bytes.of(new byte[]{1}).slice(0, 1).length());

assertEquals(0, Bytes.of(new byte[]{1,2,3}).slice(1, 1).length());

assertEquals(1, Bytes.of(new byte[]{1,2,3}).slice(1, 2).length());

assertEquals(2, Bytes.of(new byte[]{1,2,3}).slice(0, 2).length());

assertEquals(3, Bytes.of(new byte[]{1,2,3}).slice(0, 3).length());

}

@ParameterizedTest

@MethodSource("provideInputForBytesLengthTest")

void testBytesLength(String testDescription, int expectedLength, byte[] inputArray, int from, int to) {

assertEquals(expectedLength, Bytes.of(inputArray).slice(from, to).length());

}

static Stream<Arguments> provideInputForBytesLengthTest(){

return Stream.of(

Arguments.of("Length 0 for empty array", 0, new byte[]{}, 0,0),

Arguments.of("Length 0 for slice with same from and to", 0, new byte[]{1}, 0,0),

Arguments.of("Length 1 for slice with distance between from and to", 1, new byte[]{1,2,3}, 0,1),

Arguments.of("Length 0 for empty array", 0, new byte[]{1,2,3}, 1,1),

Arguments.of("Length 1 for slice with distance between from and to", 1, new byte[]{1,2,3}, 1, 2),

Arguments.of("Length 2 for slice with distance between from and to", 2, new byte[]{1,2,3}, 0, 2),

Arguments.of("Length 3 for slice with distance between from and to", 3, new byte[]{1,2,3}, 0, 3)

);

}

thanks for the suggestion. I prefer the original version more. It seems to be kinda minimalistic and more readable

fmacleal · 2024-08-07T12:20:04Z

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java

+        assertThrows(IndexOutOfBoundsException.class, () -> Bytes.of(new byte[]{}).slice(0, 0).byteAt(0));
+        assertThrows(IndexOutOfBoundsException.class, () -> Bytes.of(new byte[]{1}).slice(0, 1).byteAt(1));
+        assertThrows(IndexOutOfBoundsException.class, () -> Bytes.of(new byte[]{1}).slice(0, 1).byteAt(-1));
+        assertThrows(IndexOutOfBoundsException.class, () -> Bytes.of(new byte[]{1,2,3}).slice(1, 2).byteAt(1));


suggestion:

Break this test in two, one to test the Exceptions and other to validate the proper functionality of the method. You could also get use of the parameterized test here. HAving a single assertion with different inputs.

fmacleal · 2024-08-07T12:34:47Z

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesTest.java

+
+        Bytes bytes = Bytes.of(finalArray);
+
+        assertEquals(64, String.format("%s", bytes).length());


question:

Why the size is doubled? 😅
Because it become an object and it occupies more space? 🤔

here we're limiting printing of byte sequences to 32 bytes. In stringular / hex form it'd be 64 chars

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java

fmacleal · 2024-08-07T13:50:09Z

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java

@@ -74,4 +113,44 @@ void testEmptySlice() {
        assertEquals(0, actualResult.length());
        assertArrayEquals(expectedResult, actualResult.copyArray());
    }
+
+    private static void checkArraycopy(Functions.Action5<Object, Integer, Object, Integer, Integer> fun) {


suggestion:

Although all variables are used here, I would suggest create local variables to hold the value of the arrays. So we can have a better understanding just by reading the code withoug have to interpret it. For example:

... byte[] anotherDestArray= new byte[5]; assertArrayEquals(anotherDestArray, dest); // yet unmodified fun.apply(origin, 0, dest, 0, 5); byte[] expectedDestAfterCopy = new byte[]{1,2,3,4,5}; assertArrayEquals(expectedDestAfterCopy, dest); anotherDestArray = new byte[5]; fun.apply(origin, 1, anotherDestArray, 1, 3); byte[] newExpectedArray = new byte[]{0,2,3,4,0}; assertArrayEquals(newExpectedArray, anotherDestArray);

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesTest.java

fmacleal

LGTM

fmacleal · 2024-08-20T19:33:34Z

pipeline:run

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java

nagarev

LGTM

sonarcloud · 2024-08-21T13:46:18Z

Quality Gate passed

Issues
56 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.8% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarCloud

fmacleal

LGTM

fmacleal · 2024-08-21T14:09:43Z

pipeline:run

VaWheel · 2024-10-03T13:53:46Z

rskj-core/src/main/java/co/rsk/core/types/bytes/BytesSlice.java

+        if (newLength < 0) {
+            throw new IllegalArgumentException(from + " > " + to);
+        }
+        byte[] copy = new byte[newLength];


should copy array length also be Math.min(length() - from, newLength)? in other case it may contain zero bytes at the end which are not present in original array

@VaWheel , the idea here was to mimic the behaviour of Arrays#copyOfRange(Object[], int, int) (pls see this and this javadoc's)

… Arrays.copyOfRange; add more tests

….java Co-authored-by: Nazaret García Revetria <nazaret@iovlabs.org>

sonarcloud · 2024-10-04T11:20:49Z

Quality Gate passed

Issues
76 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.8% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarCloud

Vovchyk force-pushed the vovchyk/native-cont-impv branch from 1267e76 to 7d53a8e Compare July 26, 2024 12:57

Vovchyk marked this pull request as ready for review August 5, 2024 13:45

asoto-iov previously approved these changes Aug 7, 2024

View reviewed changes

fmacleal previously approved these changes Aug 7, 2024

View reviewed changes

Vovchyk dismissed stale reviews from fmacleal and asoto-iov via 2cb3dc6 August 9, 2024 12:33

fmacleal previously approved these changes Aug 14, 2024

View reviewed changes

asoto-iov previously approved these changes Aug 15, 2024

View reviewed changes

fmacleal dismissed stale reviews from asoto-iov and themself via b35867c August 20, 2024 16:02

fmacleal force-pushed the vovchyk/native-cont-impv branch from b35867c to b0bae39 Compare August 20, 2024 17:36

nagarev reviewed Aug 21, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest.java Outdated Show resolved Hide resolved

nagarev approved these changes Aug 21, 2024

View reviewed changes

fmacleal approved these changes Aug 21, 2024

View reviewed changes

Vovchyk force-pushed the vovchyk/native-cont-impv branch from 7ae40e4 to f243260 Compare September 30, 2024 08:34

VaWheel reviewed Oct 3, 2024

View reviewed changes

Vovchyk and others added 5 commits October 4, 2024 14:06

refactor: update Bytes and BytesSlice to 'mimic' System.arraycopy and…

5511ec4

… Arrays.copyOfRange; add more tests

perf: improve decoding in CallTransaction

acf6287

perf: improve decoding in RLP

b97230f

perf: refactor some tests

b8fdf84

Update rskj-core/src/test/java/co/rsk/core/types/bytes/BytesSliceTest…

39f9c56

….java Co-authored-by: Nazaret García Revetria <nazaret@iovlabs.org>

Vovchyk force-pushed the vovchyk/native-cont-impv branch from f243260 to 39f9c56 Compare October 4, 2024 11:06

Vovchyk merged commit 4084daf into master Oct 4, 2024
11 checks passed

Vovchyk deleted the vovchyk/native-cont-impv branch October 4, 2024 11:46

aeidelman added this to the Arrowhead 6.4.0 milestone Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: improve decoding in CallTransaction #2647

perf: improve decoding in CallTransaction #2647

Vovchyk commented Jul 26, 2024

fmacleal left a comment

fmacleal Aug 7, 2024

Vovchyk Aug 9, 2024

fmacleal Aug 7, 2024

fmacleal Aug 7, 2024

Vovchyk Aug 9, 2024

fmacleal Aug 7, 2024

fmacleal left a comment

fmacleal commented Aug 20, 2024

nagarev left a comment

sonarcloud bot commented Aug 21, 2024

fmacleal left a comment

fmacleal commented Aug 21, 2024

VaWheel Oct 3, 2024

Vovchyk Oct 3, 2024

sonarcloud bot commented Oct 4, 2024

-    @Test
-    void testBytesLength() {
-        assertEquals(0, Bytes.of(new byte[]{}).slice(0, 0).length());
-        assertEquals(0, Bytes.of(new byte[]{1}).slice(0, 0).length());
-        assertEquals(1, Bytes.of(new byte[]{1}).slice(0, 1).length());
-        assertEquals(0, Bytes.of(new byte[]{1,2,3}).slice(1, 1).length());
-        assertEquals(1, Bytes.of(new byte[]{1,2,3}).slice(1, 2).length());
-        assertEquals(2, Bytes.of(new byte[]{1,2,3}).slice(0, 2).length());
-        assertEquals(3, Bytes.of(new byte[]{1,2,3}).slice(0, 3).length());
-    }
+    @ParameterizedTest
+    @MethodSource("provideInputForBytesLengthTest")
+    void testBytesLength(String testDescription, int expectedLength, byte[] inputArray,  int from, int to) {
+        assertEquals(expectedLength, Bytes.of(inputArray).slice(from, to).length());
+    }
+    static Stream<Arguments> provideInputForBytesLengthTest(){
+        return Stream.of(
+                Arguments.of("Length 0 for empty array", 0, new byte[]{}, 0,0),
+                Arguments.of("Length 0 for slice with same from and to", 0, new byte[]{1}, 0,0),
+                Arguments.of("Length 1 for slice with distance between from and to", 1, new byte[]{1,2,3}, 0,1),
+                Arguments.of("Length 0 for empty array", 0, new byte[]{1,2,3}, 1,1),
+                Arguments.of("Length 1 for slice with distance between from and to", 1, new byte[]{1,2,3}, 1, 2),
+                Arguments.of("Length 2 for slice with distance between from and to", 2, new byte[]{1,2,3}, 0, 2),
+                Arguments.of("Length 3 for slice with distance between from and to", 3, new byte[]{1,2,3}, 0, 3)
+                );
+    }


		Bytes bytes = Bytes.of(finalArray);

		assertEquals(64, String.format("%s", bytes).length());

perf: improve decoding in CallTransaction #2647

perf: improve decoding in CallTransaction #2647

Conversation

Vovchyk commented Jul 26, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

fmacleal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmacleal left a comment

Choose a reason for hiding this comment

fmacleal commented Aug 20, 2024

nagarev left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 21, 2024

Quality Gate passed

fmacleal left a comment

Choose a reason for hiding this comment

fmacleal commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 4, 2024

Quality Gate passed