[foreign-preview] RFR: 8282061: Improve support for deterministic closure of shared scopes [v3]

[Maurizio Cimadamore]

On Mon, 21 Feb 2022 15:02:35 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix whitespaces
>
> src/hotspot/share/prims/scopedMemoryAccess.cpp line 87:
> 
>> 85:   LockFreeStackThreadsElement(JavaThread* thread) : _entry(), _thread(thread) {}
>> 86:   typedef LockFreeStack<Element, &entry_ptr> ThreadStack;
>> 87: };
> 
> 2 things in this class.
> 1. `_entry` seems to be the next entry in the stack (looking at LockFreeStack). So I'd prefer to call the field and function `_next` and `next_ptr` respectively.
> 2. The _entry() field is default initialized, which AFAIK means it might be garbage (depending on what the compiler does). Either way, please initialize it to `nullptr` explicitly.
> Suggestion:
> 
> class LockFreeStackThreadsElement : public CHeapObj<mtInternal> {
>   typedef LockFreeStackThreadsElement Element;
> 
>   Element* volatile _next;
>   static Element* volatile* next_ptr(Element& e) { return &e._next; }
> 
> public:
>   JavaThread* _thread;
>   LockFreeStackThreadsElement(JavaThread* thread) : _next(nullptr), _thread(thread) {}
>   typedef LockFreeStack<Element, &next_ptr> ThreadStack;
> };

Thanks for the suggestion

> src/hotspot/share/prims/scopedMemoryAccess.cpp line 190:
> 
>> 188:     // if the thread is not in the list handle, we can safely skip further handshakes,
>> 189:     // as that means that the thread has been created when the scope state has already
>> 190:     // been set to CLOSED - meaning that the thread access will fail anyway.
> 
> I don't get this comment...
> 
> It seems true that 
> 1. there might be threads in the thread list which are not in the problematic stack. (threads that were created after the initial handshake)
> 2. or vice versa, there can be threads in the stack that are not in the thread list. (threads that have died since the initial handshake)
> 
> ---
> 
> 1. seems to be covered by the fact that we are iterating the threads in the thread stack, instead of the ones in the list.
> 2. It seems that the `includes` check is needed since a thread might have died since the initial handshake, in which case we don't want to handshake it again?
> 
> So, I think the comment should read something regarding 2. instead. e.g. 
> Suggestion:
> 
>     // if the thread is not in the list handle, it means that it has died since the initial handshake.
>     // in that case we don't need to handshake it again.

The logic here has changed to reflect some internal comments - I will update the comment accordingly

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/643