RFR: 8291237: Encapsulate nmethod Deoptimization logic [v2]

[Dean Long]

On Mon, 1 Aug 2022 04:58:49 GMT, Axel Boldt-Christmas <duke at openjdk.org> wrote:

>> The proposal is to encapsulate the nmethod mark for deoptimization logic in one place and only allow access to the `mark_for_deoptimization` from a closure object:
>> ```C++
>> class DeoptimizationMarkerClosure : StackObj {
>> public:
>>   virtual void marker_do(Deoptimization::MarkFn mark_fn) = 0;
>> };
>> 
>> This closure takes a `MarkFn` which it uses to mark which nmethods should be deoptimized. This marking can only be done through the `MarkFn` and a `MarkFn` can only be created in the following code which runs the closure. 
>> ```C++
>> {
>>   NoSafepointVerifier nsv;
>>   assert_locked_or_safepoint(Compile_lock);
>>   marker_closure.marker_do(MarkFn());
>>   anything_deoptimized = deoptimize_all_marked();
>> }
>> if (anything_deoptimized) {
>>   run_deoptimize_closure();
>> }
>> 
>> This ensures that this logic is encapsulated and the `NoSafepointVerifier` and `assert_locked_or_safepoint(Compile_lock)` makes `deoptimize_all_marked` not having to scan the whole code cache sound.
>> 
>> The exception to this pattern, from `InstanceKlass::unload_class`, is discussed in the JBS issue, and gives reasons why not marking for deoptimization there is ok.
>> 
>> An effect of this encapsulation is that the deoptimization logic was moved from the `CodeCache` class to the `Deoptimization` class and the class redefinition logic was moved from the `CodeCache` class to the `VM_RedefineClasses` class/operation. 
>> 
>> Testing: Tier 1-5
>> 
>> _Update_
>> ---
>> Switched too using a RAII object to track the context instead of putting code in a closure. But all the encapsulation is still the same. 
>> 
>> Testing: Tier 1-7
>> 
>> _Update_
>> ---
>>> @stefank suggested splitting out unloading klass logic change into a separate issue [JDK-8291718](https://bugs.openjdk.org/browse/JDK-8291718).
>>> 
>>> Will probably also limit this PR to only encapsulation. (Skipping the linked list optimisation) And create a separate issue for that as well.
>>> 
>>> But this creates a chain of three dependent issues. [JDK-8291237](https://bugs.openjdk.org/browse/JDK-8291237) depends on [JDK-8291718](https://bugs.openjdk.org/browse/JDK-8291718). And the link list optimisation depend will depend on [JDK-8291237](https://bugs.openjdk.org/browse/JDK-8291237).
>>> 
>>> Will mark this as a draft for now and create a PR for [JDK-8291718](https://bugs.openjdk.org/browse/JDK-8291718) first.
>
> Axel Boldt-Christmas has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Add assertions
>  - Fix marked logic
>  - Erik refactorings

Now that you are building a separate list of nmethods to deoptimize, what protects that list from nmethods being recycled by other threads?  Don't you need to hold the CodeCache_lock?

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

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