RFR: 8306767: Concurrent repacking of extra data in MethodData is potentially unsafe [v24]

[Coleen Phillimore]

On Mon, 22 Jan 2024 11:14:41 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> As explained in a [comment below](https://github.com/openjdk/jdk/pull/16840#issuecomment-1833529561), we have to ensure that reading/writing/cleaning the extra data all needs to be guarded by the `extra_data_lock`, and that no safepoint should happen while holding that lock, so that the lock is not broken.
>> 
>> I introduced `check_extra_data_locked`, where I check that we hold the lock, and if we are a java thread (only those ever safepoint), that we currently are in a `NoSafepointVerifier` scope, hence we verify that no safepoint will be taken.
>> 
>> I placed `check_extra_data_locked` in all the places where we access the extra data, and then placed locks (with implicit no-safepoint-verifiers) at the call-site of those places.
>> 
>> I also needed to change the rank of `extra_data_lock` to `nosafepoint` and set the `Mutex::_no_safepoint_check_flag` when taking the lock. Otherwise I could not take the lock from a VM thread.
>> 
>> **Testing**
>> Testing: tier1-3 and stress.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cleanup unnecessary changes

This looks better.  I have a couple of small requests.  Thanks.

src/hotspot/share/oops/methodData.hpp line 2316:

> 2314:   uint arg_modified(int a)                       { // Lock and avoid breaking lock with Safepoint
> 2315:                                                    MutexLocker ml(extra_data_lock(), Mutex::_no_safepoint_check_flag);
> 2316:                                                    ArgInfoData *aid = arg_info();

Can you move these to methodData.inline.hpp so that you don't have to include mutexLocker.hpp in a header file?

src/hotspot/share/oops/methodData.hpp line 2553:

> 2551:            "JavaThread must have NoSafepointVerifier inside lock scope");
> 2552: #endif
> 2553:   }

Since this is not performance critical, can you move this to the .cpp file?

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16840#pullrequestreview-1841447172
PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1464957311
PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1464958698