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

Ty Young youngty1997 at gmail.com
Wed Nov 24 21:12:09 UTC 2021 

On 11/24/21 1:35 PM, Maurizio Cimadamore wrote:
>
> 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).
>
>

Hopefully everyone can agree that there is a difference between 
redefining(violating?) already existing, long established terminology 
and taking cosmetic liberties with API design. I have no idea what Uwe 
needs unaligned access for(uses less memory?) but I can't see how the 
current alignment scheme makes sense. If someone wanted to do something 
different then they are probably in the special use case situation, not 
the other way around.


What constants someone might be interested in seems kind of dependent on 
if they are only interested in off-heap memory for use in Java or 
working with native code. Regardless of what's exposed, how is someone 
supposed to use a VarHandles when the layout that you use to create a 
MemorySegment has a mutable meaning? Using a long handle on a 
MemorySegment created using the ValueLayout of an int just causes an 
index out of bounds exception, for example.


It's not pretty, but for what it's worth, best thing I can think of is 
to just define types like INT8, INT16, etc. Maybe anything other than 
the native order should be considered a niche just like overriding bit 
alignment.


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


Not sure if I'm explicitly using upcalls myself. I'm trying to use a 
mapped memory file as an "accounting allocator"(reports memory 
allocated, number of allocations) which gets my app to launch but then 
it will randomly start reporting strange values, typically data that 
comes from struct fields. Either memory corruption, or bytes are being 
shared between segments.


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

More information about the panama-dev mailing list