On Thu, 9 Nov 2023 15:34:38 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> src/java.base/share/classes/java/lang/foreign/Linker.java line 792:
>>
>>> 790: * @param allowHeapAccess whether the linked function should
>>> allow access to the Java heap.
>>> 791: */
>>> 792: static Option critical(boolean allowHeapAccess) {
>>
>> Speaking of public API, I'm surprised to see critical function property
>> conflated with ability to perform on-heap accesses. These aspects look
>> orthogonal to me. Any particular reason not to represent them as 2 separate
>> `Option`s?
>>
>> Even though it's straightforward to support on-heap accesses during critical
>> function calls, object pinning would support that for non-critical function
>> calls as well, but proposed API doesn't cover it and new API will be
>> required. What's the plan here?
>
>> Even though it's straightforward to support on-heap accesses during critical
>> function calls, object pinning would support that for non-critical function
>> calls as well, but proposed API doesn't cover it and new API will be
>> required. What's the plan here?
>
> The issue is that most GCs don't support object pinning. I don't think we
> want an API that only works for some GCs. But, if we do, there's a better API
> that we can have for just pinning support, which is a
> `MemorySegment::pin(Arena)` method that returns a MemorySegment wrapping the
> pinned array. That would allow doing multiple native calls with just a single
> pin operation, and also allows embedding pointers to pinned segments in other
> data structures.
>
> For the current approach where we make the array accessible for the duration
> of the native call: without pinning support, other GCs would have to use
> GCLocker. That means that the native call also has to be relatively
> short-lived, at which point I figured we might as well drop the thread state
> transition, since that has the same requirement. I.e. we detect that the call
> is short-lived, and do the optimization ourselves without the hint from the
> user (`critical`). This coincidentally also greatly simplifies the
> implementation. In a prior iteration I did have a separate `allowHeap`
> `Option` that implied `critical`. But it was suggested to just merge the two
> together in that case.
I stand by the current design: a GCLocker-based mechanism (as the current
implementation is) needs to have similar restrictions both on-heap access and
also removal of state transitions. It's true that a more general notion of
pinning is possible, which doesn't necessarily require special support from the
linker (because we can turn an heap segment into a native segment by pinning it
and _then_ pass that to the linker). But at this point in this support for
region-based pinning is not mature enough to justify such an API (and, if we'll
ever get to that point, that would not invalidate the critical linker options).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16201#discussion_r1388411265
