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

Wed Nov 24 16:53:43 UTC 2021

(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://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L563 
>>
>>
>> https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L573 
>>
>>
>> https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L583 
>>
>>
>> https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L594 
>>
>>
>> https://github.com/openjdk/jdk/blob/cf7adae6333c7446048ef0364737927337631f63/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L604 
>>
>>
>>
>> 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://github.com/openjdk/panama-foreign/blame/c15cb9c11e03c7552d2143273959acac30969db7/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java#L560 
>>
>>
>>
>> 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://github.com/rmaucher/openssl-panama-foreign

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