Optimisation for UInt256.fromBytesBE #9547
Conversation
thomas-quadratic
force-pushed
the
feat/fromBytesBE-optim
branch
from
ced26d3 to
c760630
Compare
Before, limbs were stored in little-endian. But to use Arrays.mismatch to our advantage, it is better to have it big-endian. This commit makes UInt256.java big-endian in limbs. We still need to migrate all tests and benchmark. Signed-off-by: Thomas Zamojski <[email protected]>
Also added tests that were failing and now pass. Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Thomas Zamojski <[email protected]>
Before, limbs were stored in little-endian. But to use Arrays.mismatch to our advantage, it is better to have it big-endian. This commit makes UInt256.java big-endian in limbs. We still need to migrate all tests and benchmark. Signed-off-by: Thomas Zamojski <[email protected]>
Also added tests that were failing and now pass. Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Thomas Zamojski <[email protected]>
lu-pinto
force-pushed
the
feat/fromBytesBE-optim
branch
from
c760630 to
e324e55
Compare
|
|
||
| private final int[] limbs; | ||
| private final int length; | ||
| private final int offset; |
There was a problem hiding this comment.
please add a comment as to what this offset means. Above it is still mentioned length
| int limb = 0; | ||
| int i = N_LIMBS - 1; // Index in int array | ||
| int b = bytes.length - 1; // Index in bytes array | ||
| int limb; |
There was a problem hiding this comment.
nit: move limb definition inside loop
| return new UInt256(limbs, len); | ||
| int offset = N_LIMBS - len; | ||
| System.arraycopy(arr, 0, limbs, offset, len); | ||
| return new UInt256(limbs, offset); |
There was a problem hiding this comment.
not guaranteed that the offset is the real offset. For instance if you have int[] arr = new int[]{0,0,1,2} you will have offset = 0 and it should be offset = 2. This is important because you are constructing a new UInt256 so it should follow same guarantees as fromBytesBE
| * @return Big-endian ordered bytes for this UInt256 value. | ||
| */ | ||
| public byte[] toBytesBE() { | ||
| public byte[] toBytesBEOld() { |
There was a problem hiding this comment.
why do we have this on? Just for benchmarks? Remove after PR is sound.
| */ | ||
| public byte[] toBytesBE() { | ||
| byte[] result = new byte[BYTESIZE]; | ||
| for (int i = this.limbs.length - N_LIMBS, j = 0; i < this.limbs.length; i++, j += 4) { |
There was a problem hiding this comment.
could use offset to shorten the for loop
There was a problem hiding this comment.
never mind... I get it now, we cannot trust it after computations.
| public boolean isZero() { | ||
| return (limbs[0] | limbs[1] | limbs[2] | limbs[3] | limbs[4] | limbs[5] | limbs[6] | limbs[7]) | ||
| == 0; | ||
| return Arrays.mismatch(limbs, ZERO.limbs) == -1; |
There was a problem hiding this comment.
probably the older impl was more performant though.
| if (comp != 0) return comp; | ||
| } | ||
| return 0; | ||
| int i = Arrays.mismatch(a.limbs, b.limbs); |
There was a problem hiding this comment.
faster path is to check on offset?
if (a.offset != b.offset) return Integer.compare(b.offset, a.offset);
Then run mismatch on remainder.
There was a problem hiding this comment.
Actually there's Arrays.compareUnsigned already if you want to use it
There was a problem hiding this comment.
ok again I get why you did it, cause you cannot trust offset after computations.
| | (this.limbs[6] ^ other.limbs[6]) | ||
| | (this.limbs[7] ^ other.limbs[7]); | ||
| return xor == 0; | ||
| return Arrays.mismatch(this.limbs, other.limbs) == -1; |
There was a problem hiding this comment.
There's also Arrays.equals() that is vectorized and simplifies the code.
| absInto(x, this.limbs, N_LIMBS); | ||
| absInto(y, modulus.limbs, N_LIMBS); | ||
| System.arraycopy(this.limbs, 0, x, 0, N_LIMBS); | ||
| System.arraycopy(modulus.limbs, 0, y, 0, N_LIMBS); |
There was a problem hiding this comment.
why not offset instead of src=0?
| int divLen = effectiveLength(dividend); | ||
|
|
||
| // Shortcut: if dividend < modulus or dividend == modulus | ||
| int cmp = compareLimbs(dividend, modulus); |
There was a problem hiding this comment.
Why not using modLen and divLen in compareLimbs to short circuit on size?
| this.offset = offset; | ||
| } | ||
|
|
||
| UInt256(final int[] limbs) { |
There was a problem hiding this comment.
either remove this constructor or make sure there's a guarantee on offset
| // -------------------------------------------------------------------------- | ||
|
|
||
| UInt256(final int[] limbs, final int length) { | ||
| UInt256(final int[] limbs, final int offset) { |
There was a problem hiding this comment.
make this constuctor private not just package private
| public UInt256 mod(final UInt256 modulus) { | ||
| if (this.isZero() || modulus.isZero()) return ZERO; | ||
| return new UInt256(knuthRemainder(this.limbs, modulus.limbs), modulus.length); | ||
| return new UInt256(knuthRemainder(this.limbs, modulus.limbs)); |
There was a problem hiding this comment.
I see why you are using the the UInt256(int[]) constructor - basically when you don't care about the result because the UInt256 is not going to be reused...
I would still lean more towards removing that constructor as it's not easy to read unless I check in the place where it is used.
| int[] sum = addImpl(this.limbs, other.limbs); | ||
| int[] rem = knuthRemainder(sum, modulus.limbs); | ||
| return new UInt256(rem, modulus.length); | ||
| return new UInt256(rem, rem.length - modulus.limbs.length + modulus.offset); |
There was a problem hiding this comment.
nit: not a big deal since UInt256 will be discarded but I cannot understand the formula to compute offset
| // } | ||
|
|
||
| // Comparing two int subarrays as big-endian multi-precision integers. | ||
| private static int compareLimbs(final int[] a, final int[] b) { |
There was a problem hiding this comment.
this can be as simple as I believe:
private static int compareLimbs(final int[] a, final int aLen, final int[] b, final int bLen) {
if (aLen != bLen) {
return Integer.compare(aLen, bLen);
}
int offset = a.length - ALen;
int index = Arrays.mismatch(a, offset, a.length, b, offset, a.length);
if (index == -1) {
return 0;
}
return Integer.compareUnsigned(a[index], b[index]);
}
| // // Unchecked : xLen <= x.length, xLen <= N_LIMBS | ||
| // int i = Arrays.mismatch(x, offset, length, ZERO.limbs, 0, N_LIMBS); | ||
| // return N_BITS_PER_LIMB * i + Integer.numberOfLeadingZeros(x[offset + i]); | ||
| // } |
lu-pinto
left a comment
lu-pinto
left a comment
There was a problem hiding this comment.
it's functionally OK but I would remove some dead and commented out code.
If you don't want to address performance callouts right now, I can do that in my PR later no worries at all, so feel free to go ahead an merge once it's cleaned
2 participants
PR description
@ahamlat improved the performance of UInt256.fromBytesBE using loops.
Motivated by that finding and the migration to big-endian limbs, we propose further improvements in this PR.
Notably, the implementation could use Arrays.mismatch to deal with 0 padding effectively, which was missing previously.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests