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

[Roland Westrelin]

On Mon, 15 Jan 2024 10:23:21 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> @rwestrel I still believe this is safe. But maybe also ugly.
>> 
>> I looked into making the locking more fine-grained, so that we could avoid unlocking the lock temporarily.
>> The biggest problem is in `ciMethodData::load_remaining_extra_data`. Here we first (iteratively) clean, and then assume that we still hold the lock when we copy it for the `ciMethodData`. Hence, it seems the lock has to be held at this outer scope, but then temporarily unlocked to allow calls to `get_method` in `PrepareExtraDataClosure::finish`.
>
> Alternatives to make it prettier:
> Make `prepare_metadata` lock, and pass out an object that holds that lock, i.e. widen the scope of the `MutexLocker`. Maybe this can be done with return-value-optimization? But I'm not sure this is a great idea. Another idea @chhagedorn and I thought about was having some Locker object that you can call lock/unlock on, repeatedly. But once the Locker goes out of scope, it checks if it is in the locked state, and only unlocks then. Or maybe it asserts that it is in the locked state, and then unlocks.
> 
> Because essencially we need to allow the retry-logic to unlock in between tries. But we also still need to access the `uncached_methods` array that is filled inside the locked region.
> 
> I'm not sure such a refactoring is worth it. Let me know what you think @rwestrel

Given that logic existed, I think you can leave it as is but a comment that explains why it is safe would be useful.

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

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