RFR: 8297168: Provide a bulk OopHandle release mechanism with the ServiceThread [v3]

[Daniel D. Daugherty]

On Mon, 21 Nov 2022 23:00:30 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> When a `JavaThread` terminates it has to release all `OopHandles` that it uses. This can't be done by the thread itself due to timing issues, so it is handed-off to the `ServiceThread` to do it - ref [JDK-8244997](https://bugs.openjdk.org/browse/JDK-8244997).
>> 
>> Initially there was only one `OopHandle` to handle - that of the `threadObj`, but since Loom there are another 3 `OopHandles` to process. The existing logic does the hand-off one `OopHandle` at a time but that is a potential synchronization bottleneck because each hand-off acquires the `ServiceLock`, enqueues the `OopHandle`, issues a notify to (potentially) wakeup the `ServiceThread` and then releases the `ServiceLock`. This can lead to high contention on the `ServiceLock` and also bad scheduling interactions with the `ServiceThread`.
>> 
>> This PR contains two commits. The first simply changes the API so that we pass the target `JavaThread` so that all 4  `OopHandles` can be extracted in one call. The second commit looks at streamlining things further by consolidating  into a single `OopHandleList` instance.
>> 
>> As `ServiceThread is a subclass of `JavaThread` I wasn't concerned about it having detailed knowledge of the JavaThread implementation.
>> 
>> Testing:
>>   - tiers 1-3
>>   - checked still no memory leak (ref [JDK-8296463](https://bugs.openjdk.org/browse/JDK-8296463))
>> 
>> Thanks.
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Move all the code to JavaThread - as requested by @coleenp.

Thumbs up!

Sorry I didn't submit my review on-time...

src/hotspot/share/runtime/javaThread.cpp line 602:

> 600: JavaThread::~JavaThread() {
> 601: 
> 602:   // Enqueue OopHandles for release by the service thread.

nit - s/service thread/ServiceThread/

src/hotspot/share/runtime/javaThread.hpp line 1182:

> 1180:   // List of OopHandles to be released - guarded by the Service_lock.
> 1181:   static OopHandleList* _oop_handle_list;
> 1182:   // Add our OopHandles to the list for the service thread to release.

nit - s/service thread/ServiceThread/

src/hotspot/share/runtime/serviceThread.hpp line 53:

> 51:   bool is_service_thread() const                 { return true; }
> 52: 
> 53:   // Add event to the service thread event queue.

nit - s/service thread/ServiceThread/
Not your typo, but since you're close by...

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

PR: https://git.openjdk.org/jdk/pull/11254