On Mon, 20 Jan 2025 15:03:37 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> Matthias Ernst has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implementation notes.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/CallBufferCache.java
> line 97:
>
>> 95: public static long acquire() {
>> 96: // Protect against vthread unmount.
>> 97: Continuation.pin();
>
> Other code that does this, first checks if we are in a virtual thread.
> Note that we have two options for dealing with problematic cases:
> * pin/unpin as soon as we see a virtual thread
> * if we have a virtual thread and we detect a race in accessing the cache
> (e.g. because of how the virtual threads have been scheduled to the same
> underlying carrier thread), we could avoid pinning, but just allocate a fresh
> new segment (thus avoiding recycling issues)
Would be worth understanding why we would _not_ want to pin here.
IINM, the `pin` code itself
[checks](https://github.com/mernst-github/jdk/blob/f5573f5cbdcae5d1303c8b58d2946c168b977326/src/hotspot/share/opto/library_call.cpp#L3754)
whether there's continuation, what would we win by a redundant `if` ?
Second, I'm not sure how you would want to detect a race, if you cannot trust
any field you just read.
I can't see how you would get around using atomics, which is a much more
expensive alternative.
I designed these to be as short sections as possible (if available, take/null
out slot ; if free, put slot; ). If that isn't what pinning is good for, I'm
not sure what is.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1922661817
