perf: improve mem reading in Memory #2667
Conversation
|
There was a problem hiding this comment.
Nice job. I have a few comments, mostly cosmetics and nitpicks. Doesn't prevent the approval.
Question, I didn't see any test regarding some classes you created. Like the ones to convert to HexString or the arraycopy on MemorySlice. Do we already have these operations covered in other tests and wasn't necessary to add one?
rskj-core/src/main/java/co/rsk/core/types/bytes/HexPrintableBytes.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/core/types/bytes/HexPrintableBytes.java
Outdated
Show resolved
Hide resolved
| return toHexString(0, length()); | ||
| } | ||
|
|
||
| default String toHexStringV2(int off, int length) { |
There was a problem hiding this comment.
Why do we need this v2 method? Can not we just use the default String toHexString(int off, int length) method?
There was a problem hiding this comment.
just to emphasise that this is another version comparing to ByteUtil#toHexString(), which uses a 3rd-party lib. Added javadoc here
There was a problem hiding this comment.
also added jmh test here
| @@ -128,6 +128,42 @@ default Bytes copyBytes() { | |||
| default BytesSlice slice(int from, int to) { | |||
| return new BytesSliceImpl(this, from, to); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
What do you think about convert this class to an abstract class? I think it could give us various advantages:
- We could make both equals and hashcode methods part of the class and not implemented as static.
- We can force the usage of
BoundaryUtils.checkArrayIndexParam(length(), index) - Stop using default methods for defining common implementations and facilitate the testing.
There was a problem hiding this comment.
I'd rather use interface here. I guess it's quite similar to CharSequence interface that's being used for chars without concrete implementation of equals and hashCode
Vovchyk
changed the base branch from
vovchyk/rlp-impv
to
vovchyk/native-cont-impv
Vovchyk
force-pushed
the
vovchyk/mem-impv
branch
from
d3d49aa to
97593a2
Compare
Vovchyk
force-pushed
the
vovchyk/native-cont-impv
branch
from
f243260 to
39f9c56
Compare
Vovchyk
dismissed stale reviews from asoto-iov and fmacleal
The base branch was changed.
Vovchyk
force-pushed
the
vovchyk/mem-impv
branch
from
97593a2 to
5842372
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -181,21 +185,20 @@ | |||
| } | |||
|
|
|||
| /** @see org.ethereum.crypto.cryptohash.Digest */ | |||
| public void update(byte[] input) | |||
| public void update(BytesSlice input) | |||
Check notice
Code scanning / CodeQL
Missing Override annotation Note
| } | ||
|
|
||
| /** @see org.ethereum.crypto.cryptohash.Digest */ | ||
| public void update(byte[] input, int offset, int len) | ||
| public void update(BytesSlice input, int offset, int len) |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
Vovchyk
force-pushed
the
vovchyk/mem-impv
branch
2 times, most recently
from
dfaf622 to
bc1b4b6
Compare
Vovchyk
force-pushed
the
vovchyk/mem-impv
branch
from
bc1b4b6 to
4aef18d
Compare
Vovchyk
force-pushed
the
vovchyk/mem-impv
branch
from
4aef18d to
e1d6fd4
Compare
|
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: