Integrated: 8275063: Implementation of Foreign Function & Memory API (Second incubator)

Wed Nov 24 19:35:50 UTC 2021

On 24/11/2021 17:52, Ty Young wrote:
> According to Google search results, the ability to do non-aligned 
> memory access seems to be purely CPU architecture dependent and, even 
> if allowed(which it is on x86), it comes at a performance penalty. 
> Again, according to Google search results, an address is only ever 
> aligned if ADDRESS % TYPE_ALIGNMENT == 0 where the alignment for an 
> int is the same as its size in bytes(4), which has been my 
> understanding this whole time and, IIRC, the API would spit out a 
> misaligned memory access error before these changes if that wasn't the 
> case.
>
>
> The ability to read/write to misaligned memory is fine, but saying an 
> INT is 1-byte aligned seems to be factually incorrect. Is setting the 
> correct alignments and not checking for aligned memory access not 
> possible?

It is possible to set correct alignment and then have unaligned access 
too. But you need different layouts for aligned and unaligned access. 
The constant we provide are designed for maximum flexibility (which, as 
you point out, might sometimes lead to abuse/misuse).

In general we have 3 options:

1. expose only aligned constants (like it was in 17)
2. expose only unaligned constant (like it is now)
3. expose both

In reality you can always derive an aligned constant from an unaligned 
one (and viceversa), so all three approaches are fine, really.

We switched from (1) to (2) based on some feedback we received (I 
believe it was from Uwe) where it seemed like unaligned access was more 
like the norm when working with big slab of data.

In general, I believe that, no matter the polarity, somebody is going to 
be unhappy, unless we do (3). And even if we do (3) somebody could ask: 
what about BIG_ENDIAN/LITTLE_ENDIAN variants?

I really hope this doesn't mean we need to expose 8 * 2 * 2 constants, 
but I'm open to suggestions as to what set of constants people think it 
would be useful to expose. This is not a big change.

Btw, note that even in 17, JAVA_DOUBLE and JAVA_LONG were not really 
8-byte aligned, but were ADDRESS aligned (e.g. in 32-bit VMs longs and 
doubles can be 4 byte aligned, not 8).

>
> I only came across this a few days ago after spending hours trying to 
> debug memory corruption issues in my own projects that I still haven't 
> figured out by dumping the amount of bytes being allocated, alignment, 
> etc only to discover that the alignment of everything just prints 1.

If you are testing on the panama repo, there is an issue with upcall 
handling - which is fixed in the patch we integrated 18 (we will merge 
the fix soon). That issue was causing spurious memory corruption.

Maurizio

>
>
> Side note: if misaligned memory access is going to be allowed then 
> maybe a JVM property should be added to enforce memory alignment even 
> on architectures that allow it.
>
>
>
> On 11/24/21 10:53 AM, Maurizio Cimadamore wrote:
>> (dropping build-dev)
>>
>> Hi,
>> as always the general tone of the email is off in many ways. I'll try 
>> to respond to some of the points.
>>
>>
>> On 24/11/2021 16:25, Maurizio Cimadamore wrote:
>>> I think this discussion should go either on core-libs or on 
>>> panama-dev. build-dev is not the place for it. I'm addding 
>>> panama-dev - and then I'll reply from there.
>>>
>>> Maurizio
>>>
>>> On 24/11/2021 16:15, Ty Young wrote:
>>>> Was this code actually reviewed or even used/tested at all? All 
>>>> "Java" ValueLayouts have an 8-bit alignment, even if they are 
>>>> 2-byte, 4-byte, or 8-byte in size:
>>
>> Yes. I think it was reviewed and tested. Not just by us but also from 
>> the various projects which started to use it.
>>
>> The decision to make all JAVA_XYZ constants byte-aligned was a 
>> pragmatic one. All the dereference APIs take a byte offset - but if 
>> you want to dereference an `int` in a random, non-aligned memory 
>> location, JAVA_INT will not work if JAVA_INT is 4 byte aligned.
>>
>> So we decided to make alignment of these constants to be "loose" 
>> which means these layouts work in any dereference scenario. Since 
>> these layout constants are not magic in any way, other aligned 
>> constants can be defined - you don't have to use these if you don't 
>> like them.
>>
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java*L563__;Iw!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxP_6RFxQI$ 
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java*L573__;Iw!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxPLQTWa4Y$ 
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java*L583__;Iw!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxPUW9AYXs$ 
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java*L594__;Iw!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxPOVK7jco$ 
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java*L604__;Iw!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxPas1qRiQ$ 
>>>>
>>>>
>>>>
>>>> The doc for ValueLayout says:
>>>>
>>>>
>>>>  "The layout constants in this class make implicit alignment and 
>>>> byte-ordering assumption: all layout
>>>>  constants in this class are byte-aligned"
>> Yes, this says that basically there's no alignment constraint 
>> associated to these constant. Note that withBitAligned(8) and 
>> byte-aligned are the same thing. Which means no alignment.
>>>>
>>>>
>>>> Which is not true in the code itself. All of the docs for the 
>>>> constants aren't correct, either. This results in a struct that 
>>>> *should* have a byte alignment of 8 actually have 1:
>> "Not true in the code itself", "docs aren't correct" - what are you 
>> trying to say? What bit of javadoc is incorrect? Or is "incorrect" in 
>> the sense that "it doesn't do what I expect it to do" ?
>>>>
>>>>
>>>> GroupLayout layout = MemoryLayout.structLayout(
>>>>                 ValueLayout.JAVA_INT,
>>>>                 MemoryLayout.paddingLayout(32),
>>>>                 ValueLayout.JAVA_LONG);
>>>>
>>>> System.out.println(layout.byteAlignment()); // prints 1!
>>
>> Yes, because JAVA_INT has alignment of 1 (in bytes), JAVA_LONG also 
>> has an alignment of 1 (in bytes), padding always has a layout of 1 
>> (in bytes) so overall alignment (in bytes) is 1.
>>
>> This seems to be compatible with what the javadoc says about how the 
>> constants are.
>>
>>
>>>>
>>>>
>>>> Git blame shows that these issues were introduced a whole 2 months 
>>>> ago as part of the "beautiful" and "perfect" API changes:
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://github.com/openjdk/panama-foreign/blame/c15cb9c11e03c7552d2143273959acac30969db7/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java*L560__;Iw!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxPt8Y3Yn4$ 
>>>>
>>>>
>>>>
>>>> and no one noticed? There should probably be alignment validation 
>>>> checks in place to catch this. The fact that constants are declared 
>>>> at the bottom instead of the top probably didn't help in making 
>>>> this issue more noticeable.
>>
>> I'll skip over the sarcastic quotes (but it doesn't help your cause 
>> doing that).
>>
>> What I find worrisome is that this email has been sent _after_ the 
>> code has been integrated. If you had genuine concerns over the 
>> alignment issues, why waiting _after_ said changes have been 
>> integrated? That's why we have a JEP process - Mark sent an email a 
>> week ago [1] asking for objections (in addition to the fact that we 
>> also have a public PR on github on which you can comment). So, if 
>> what you think are "issues" ended up being integrated, as you say 2 
>> months after first being introduced, I think it's a bit cheap to try 
>> and blame people working on this project for lack of review and test?
>>
>> As I explained above, this was a deliberate change - which works well 
>> with the dereference API we have. So, from our perspective, things 
>> have been tested properly - and everything has happened through a 
>> very open process. For instance, few weeks ago we learned that Tomcat 
>> was also testing the Java 18 bits [1] - and that testing process 
>> uncovered some issues in the upcall support, which we have rectified.
>>
>> Maybe we need two sets of constants, aligned and unaligned? Would 
>> that address your concern?
>>
>> Maurizio
>>
>> [1] - 
>> https://urldefense.com/v3/__https://github.com/rmaucher/openssl-panama-foreign__;!!ACWV5N9M2RV99hQ!a1C1m4R7w_at55VXOIwKBSatP_3VH9jhl5Z6O0Ct48LgUyGzKL-WE0j4fOXoaUxP8uXm9PU$ 
>>
>>
>>>>
>>>>
>>>> While I'm here, is passing ByteOrder.nativeOrder() to every 
>>>> constructor needed?
>>>>
>>>>
>>>> On 11/24/21 5:55 AM, Maurizio Cimadamore wrote:
>>>>> On Tue, 12 Oct 2021 11:16:51 GMT, Maurizio Cimadamore 
>>>>> <mcimadamore at openjdk.org> wrote:
>>>>>
>>>>>> This PR contains the API and implementation changes for JEP-419 
>>>>>> [1]. A more detailed description of such changes, to avoid 
>>>>>> repetitions during the review process, is included as a separate 
>>>>>> comment.
>>>>>>
>>>>>> [1] - https://openjdk.java.net/jeps/419
>>>>> This pull request has now been integrated.
>>>>>
>>>>> Changeset: 96e36071
>>>>> Author:    Maurizio Cimadamore <mcimadamore at openjdk.org>
>>>>> URL: 
>>>>> https://git.openjdk.java.net/jdk/commit/96e36071b63b624d56739b014b457ffc48147c4f
>>>>> Stats:     14700 lines in 193 files changed: 6958 ins; 5126 del; 
>>>>> 2616 mod
>>>>>
>>>>> 8275063: Implementation of Foreign Function & Memory API (Second 
>>>>> incubator)
>>>>>
>>>>> Reviewed-by: erikj, psandoz, jvernee, darcy
>>>>>
>>>>> -------------
>>>>>
>>>>> PR: https://git.openjdk.java.net/jdk/pull/5907