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.

Sign up for GitHub

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

Remove indify string concatenation on Inet::getInet4Address #93

Closed
wants to merge 1 commit into from
Closed

Remove indify string concatenation on Inet::getInet4Address #93

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/main/java/org/wildfly/common/net/Inet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,6 @@ public static <T extends InetAddress> T[] getAllAddressesByNameAndType(String ho
* @return the address (not {@code null})
*/
public static Inet4Address getInet4Address(int s1, int s2, int s3, int s4) {
byte[] bytes = new byte[4];
Assert.checkMinimumParameter("s1", 0, s1);
Assert.checkMaximumParameter("s1", 255, s1);
Assert.checkMinimumParameter("s2", 0, s2);
Expand All @@ -386,18 +385,47 @@ public static Inet4Address getInet4Address(int s1, int s2, int s3, int s4) {
Assert.checkMaximumParameter("s3", 255, s3);
Assert.checkMinimumParameter("s4", 0, s4);
Assert.checkMaximumParameter("s4", 255, s4);
byte[] bytes = new byte[4];
bytes[0] = (byte) s1;
bytes[1] = (byte) s2;
bytes[2] = (byte) s3;
bytes[3] = (byte) s4;
// pre-compute the digits required
Copy link
Member

Choose a reason for hiding this comment

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

You could avoid this (and some redundant branching) with something like:

    byte[] hostBytes = new byte[15];
    int cnt = encodeUnsignedByte(s1, hostBytes, 0);
    hostBytes[cnt++] = '.';
    cnt += encodeUnsignedByte(s2, hostBytes, cnt);
    ...
    String hostName = new String(hostBytes, 0, cnt, StandardCharsets.ISO_8859_1);

and then:

private static int encodeUnsignedByte(int value, byte[] bytes, int offset) {
    if (value >= 100) {
        bytes[offset++] = '0' + value / 100;
        value %= 100;
    }
    if (value >= 10) {
        ...
    }
    return offset;
}

Copy link
Author

@franz1981 franz1981 Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The redundant branching are there to avoid data dependency among encodeUnsignedByte calls, which could be indipendent from each others thanks to the upfront estimation. But is a minor: I'm opened to change it if you prefer it.

for

String hostName = new String(hostBytes, 0, cnt, StandardCharsets.ISO_8859_1);

there's an opened JDK bug (I've provided the reproducer some time ago) which show that it perform very bad, still, see https://bugs.openjdk.org/browse/JDK-8295496

quoting it

I also included String(byte[], int, int, ISO_8859_1) which disappointingly lags slightly behind the deprecated String(byte[], int, int, int).

That's why I didn't used it.

Copy link
Author

@franz1981 franz1981 Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Additionally, I have sized correctly the byte array and not using any cnt variable to simplify the life of the JDK stub generator and the bound checks generation, there, which proved to be the reason behind the bad performance of https://bugs.openjdk.org/browse/JDK-8295496

int digitsForS1 = s1 < 10 ? 1 : s1 < 100 ? 2 : 3;
int digitsForS2 = s2 < 10 ? 1 : s2 < 100 ? 2 : 3;
int digitsForS3 = s3 < 10 ? 1 : s3 < 100 ? 2 : 3;
int digitsForS4 = s4 < 10 ? 1 : s4 < 100 ? 2 : 3;
byte[] hostBytes = new byte[3 + digitsForS1 + digitsForS2 + digitsForS3 + digitsForS4];
// use encodeUnsignedByte to encode s1,s2,s3,s4 into hostBytes
encodeUnsignedByte(s1, hostBytes, 0, digitsForS1);
hostBytes[digitsForS1] = '.';
encodeUnsignedByte(s2, hostBytes, digitsForS1 + 1, digitsForS2);
hostBytes[digitsForS1 + digitsForS2 + 1] = '.';
encodeUnsignedByte(s3, hostBytes, digitsForS1 + digitsForS2 + 2, digitsForS3);
hostBytes[digitsForS1 + digitsForS2 + digitsForS3 + 2] = '.';
encodeUnsignedByte(s4, hostBytes, digitsForS1 + digitsForS2 + digitsForS3 + 3, digitsForS4);
String hostName = new String(hostBytes, 0);
Copy link
Member

Choose a reason for hiding this comment

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

You can use new String(byte[],...,StandardCharsets.ISO_8859_1) and it should be similar since it uses the same hot path with the charset is that one.

Copy link
Author

Choose a reason for hiding this comment

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

explained at #93 (comment)

try {
return (Inet4Address) InetAddress.getByAddress(s1 + "." + s2 + "." + s3 + "." + s4, bytes);
return (Inet4Address) InetAddress.getByAddress(hostName, bytes);
} catch (UnknownHostException e) {
// not possible
throw new IllegalStateException(e);
}
}

private static void encodeUnsignedByte(int value, byte[] bytes, int offset, int digits) {
assert value >= 0 && value <= 255 && digits >= 1 && digits <= 3;
if (digits == 3) {
bytes[offset + 2] = (byte) ('0' + (value % 10));
Copy link
Author

Choose a reason for hiding this comment

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

@dmlloyd FYI I didn't used the manual "strength reduction" to bit-shifts because:

  • while the code is interpreted it seems there's no need, is already pretty slow for others reasons
  • while the code is hot (C2, mostly), the JIT perform the same exact optimization

value /= 10;
}
if (digits == 2) {
bytes[offset + 1] = (byte) ('0' + (value % 10));
value /= 10;
}
bytes[offset] = (byte) ('0' + (value % 10));
}

/**
* Get an IPv6 address from eight integer segments. Each segment must be between 0 and 65535 ({@code 0xffff}).
*
Expand Down