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

[Emanuel Peter]

On Thu, 11 Jan 2024 13:43:25 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed typo
>
> src/hotspot/share/ci/ciMethodData.cpp line 96:
> 
>> 94:     // a safepoint. We temporarily release the lock and allow
>> 95:     // safepoints, and revert that at the end of the scope:
>> 96:     MutexUnlocker mu(_mdo->extra_data_lock(), Mutex::_no_safepoint_check_flag);
> 
> Why is this safe?

We only do this in the `finish` method, where we hold no reference to any profiled-data anymore. We only really need to hold the lock during `clean_extra_data` and `is_live`. But after those are done, we can quickly release the lock so that we can call `get_method`.
Does that make sense to you? I'm not super happy with the general pattern here... I basically kept the old pattern.

I wonder, maybe there is a way to move the scope of the lock, such that we only need to lock inside of `clean_extra_data`, and do not hold it before we enter `clean_extra_data`. Do you think that would be preferable?

> src/hotspot/share/ci/ciMethodData.cpp line 135:
> 
>> 133: 
>> 134:   // Lock to read ProfileData, and ensure lock is not unintentionally broken by a safepoint
>> 135:   MutexLocker ml(mdo->extra_data_lock(), Mutex::_no_safepoint_check_flag);
> 
> Is there anyway to have MutexLocker take care of verifying that the there's no safepoint? It would be nice to replace:
> 
> 
> MutexLocker ml();
> NoSafepointVerifier no_safepoint;
> 
> 
> by: 
> 
> 
> MutexLocker ml();
> 
> 
> only.

@fisk @tkrodriguez what do you suggest for that?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1449071256
PR Review Comment: https://git.openjdk.org/jdk/pull/16840#discussion_r1449072373