Skip to content
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

Numeric changes #1231

Merged
merged 3 commits into from
Aug 17, 2020
Merged

Numeric changes #1231

merged 3 commits into from
Aug 17, 2020

Conversation

camembertelectrique
Copy link
Contributor

What does this PR do?

Speed up byte array to hex string conversion

Where should the reviewer start?

Method toHexString() reimplemented

Why is it needed?

Two orders of magnitude faster implementation

@conor10
Copy link
Contributor

conor10 commented Jun 23, 2020

Thanks for taking the time to submit this.

Whilst I appreciate it is a speedup, is this sort of optimisation really necessary at present in Web3j given the main constraint we have is the EVM?

It would be helpful to understand what was the underlying motivation that led you to this specific change, as it sacrifices readability for performance.

@camembertelectrique
Copy link
Contributor Author

Conor, we have an app that generates non-EVM-based raw transactions to a precompiled contract that have large payloads compared to an EVM transaction, and we use Numeric.toHexString() to encode them. During testing, we found that this call created a significant slowdown in our app. This patch resulted in a two orders of magnitude reduction in compute time, and solved the performance issue we were having.


return stringBuilder.toString();
public static char[] toHexCharArray(byte[] input, int offset, int length, boolean withPrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is withPrefix still here?

Copy link
Contributor Author

@camembertelectrique camembertelectrique Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to preserve the functionality of toHexString and to avoid an interface change that might break existing applications.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but toHexCharArray is only defined in this class and could be private

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. toHexCharArray could be made private to preserve the interface as it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make this update I'm happy to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed and tested the suggestion for performance improvement from @esaulpaugh but found none. I have nothing else to add to this pull, it's good to merge if you approve

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the removal of withPrefix?

@esaulpaugh
Copy link
Contributor

esaulpaugh commented Jul 7, 2020

you can also eliminate one array access per byte with a size-256 encoding table:

final int[] ints = new int[] { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
final int leftNibbleMask = 0xF0;
final int rightNibbleMask = 0x0F;
for (int i = 0; i < ENCODE_TABLE.length; i++) {
    int leftChar = ints[(i & leftNibbleMask) >>> Byte.SIZE / 2];
    int rightChar = ints[i & rightNibbleMask];
    ENCODE_TABLE[i] = (leftChar << Byte.SIZE) | rightChar;
}
int hexPair = ENCODE_TABLE[input[offset] & 0xFF];
output[j] = (char) (hexPair >>> Byte.SIZE); // left
output[j+1] = (char) (hexPair & 0xFF); // right

resulting in a decent speedup https://github.com/esaulpaugh/headlong/blob/master/src/main/java/com/esaulpaugh/headlong/util/FastHex.java

And if you're optimizing for Java 9+ it's faster to encode to bytes and then use new String(byte[],int,int,int) because CompactStrings is enabled by default.

@stale
Copy link

stale bot commented Jul 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale no activity for 21 days and removed stale no activity for 21 days labels Jul 29, 2020
Copy link
Contributor

@rach-id rach-id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rach-id rach-id merged commit fd486e3 into hyperledger-web3j:master Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants