Remove indify string concatenation on Inet::getInet4Address

[franz1981]

In Quarkus's startup we noticed some allocations due to https://openjdk.org/jeps/280, see

which point to the string concatenation in Inet::getInet4Address.
The method could be optimized by:

• using a presized StringBuilder
• using an ad hoc method

I've chosen the latter, to avoid the StringBuilder allocation, given that we have knowledge about the parameters ranges ie [0, 255].

What could be done, better, is to save the intermediate byte[] hostBytes allocation to happen as a whole, by leveraging the default EliminateAllocationArraySizeLimit in case OpenJDK escape analysis will do its, job ie creating a fixed size buffer of < 64 bytes (the default value - eg 15 bytes is the max size we could expect for 255.255.255.255).
Obviously this is a dangerous bet, because rely on specific OpenJDK optimizations and when/if escape analysis kicks-in.

[dmlloyd]

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.

[franz1981]

explained at #93 (comment)

[dmlloyd]

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;
}

[franz1981]

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.

[franz1981]

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

[dmlloyd]

Also I would suggest instead making these changes to SmallRye Common Net - these two projects should be identical for this class, and it is my intention to deprecate this one and replace it with redirections to SmallRye Common.

[franz1981]

Also I would suggest instead making these changes to SmallRye Common Net - these two projects should be identical for this class, and it is my intention to deprecate this one and replace it with redirections to SmallRye Common.

Perfect, sending the PR there as well!

[franz1981]

@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

[franz1981]

@radcortez let me know if this help your startup allocation analysis

[franz1981]

I can close it, to favour one to https://github.com/franz1981/wildfly-common instead

[franz1981]

@radcortez that's why: this has never been merged and relying on smallrye/smallrye-common#275, but it seems 3.8.2 still use this, suggestions @radcortez @dmlloyd ?

[radcortez]

The fix is in SR Commons: smallrye/smallrye-common#279

But the delegating code from WF Common to SR Common has not been released:
#100

We can replace the WF API with the SR API in the Converter so we don't have to wait for the release (and update in Quarkus)