[foreign-memaccess+abi] RFR: 8268230: Foreign Linker API & Windows user32/kernel32: String conversion seems broken

[Jorn Vernee]

On Thu, 10 Jun 2021 13:13:09 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 302:
>> 
>>> 300:     }
>>> 301: 
>>> 302:     private static String addNullTerminator(String str) {
>> 
>> This is gonna allocate another string in an already allocation-heavy code. Wouldn't it be better to just add the correct termination sequence in the segment? I suggest running StrLen benchmark before/after to make sure string conversion performance isn't negatively impacted.
>
> I'll check the benchmark.
> 
> I looked at the CharSet API, and AFAICS the only way to get the number of bytes for the null terminator would be to create a String with the value `"\0"` then encode that and check the resulting `byte[]`. I thought going the string concat route would have better chances of being optimized.

The benchmark results are as follows:

Before:


Benchmark                        (size)  Mode  Cnt    Score    Error  Units
StrLenTest.panama_strlen_prefix       5  avgt   30  124.874 � 15.751  ns/op
StrLenTest.panama_strlen_prefix      20  avgt   30  131.683 �  6.011  ns/op
StrLenTest.panama_strlen_prefix     100  avgt   30  161.046 � 10.580  ns/op


After:

Benchmark                        (size)  Mode  Cnt    Score   Error  Units
StrLenTest.panama_strlen_prefix       5  avgt   30  130.758 � 5.691  ns/op
StrLenTest.panama_strlen_prefix      20  avgt   30  145.012 � 6.804  ns/op
StrLenTest.panama_strlen_prefix     100  avgt   30  179.992 � 6.457  ns/op


I think C2 should be able to eliminate the intermediate string, but there's still a slight regression.

> Wouldn't it be better to just add the correct termination sequence in the segment?

The problem is finding out how many bytes to add. Looking at Charset again, there really doesn't seem to be a way to get the number of bytes per character. The closest seems to be `charset.newEncoder().averageBytesPerChar()`, which I'm not sure is what we want. I'll ask around as well.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/554