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

[Jorn Vernee]

On Mon, 21 Feb 2022 15:06:53 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch improves the logic for closing shared resource scopes, by using the following algorithm:
>> 
>> 1. move the scope from ALIVE to CLOSED - no new thread can access segments associated with the scope
>> 2. do an initial handshake, to collect all threads that are accessing the scope concurrently
>> 3. if no thread is found, finish
>> 4. if some threads T1, T2 ... Tn are found, keep doing handshakes (only against these threads) then go back to (3). 
>> 
>> Note that the logic no longer require *three* states for shared segment, and also this logic always succeeds - that is the close operation can never fail because of a spurious access found during an handshake.
>> 
>> Note that the logic converges quickly, because handshaked threads are deoptimized - meaning that they will have to re-load the liveness state of the resource they are accessing (at which point they will just throw an exception).
>> 
>> Implementation-wise, when looking over the code with @fisk , we realized that it is possible for multiple threads to run the handhshake closure concurrently. To collect all the problematic thread, we used a lock free stack (which was already implemented in the hotspot code). Also, to keep problematic threads alive during multiple rounds of handshaking, we use a ThreadHandleList (this is also required to be able to handshake on a specific thread).
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix whitespaces

I'll run tests on Windows as well.

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

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.
2. or vice versa, there can be threads in the stack that are not in the thread list.

---

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.

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

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