RFR: 8291237: Encapsulate nmethod Deoptimization logic [v3]

[Axel Boldt-Christmas]

On Wed, 10 Aug 2022 15:23:46 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 20 additional commits since the last revision:
> 
>  - Initial draft new terminology
>  - Make _context_active atomic
>  - Missed merge changes
>  - Merge remote-tracking branch 'upstream/master' into JDK-8291237
>  - Merge branch 'JDK-8291718' into JDK-8291237
>  - Remove indentation
>  - Fix for shenandoah
>  - Merge remote-tracking branch 'upstream/master' into JDK-8291718
>  - Extend CodeCache::UnloadingScope to include klass unloading
>  - Remove double counting _perf_total_buckets_deallocated_count
>  - ... and 10 more: https://git.openjdk.org/jdk/compare/443af4e1...9f6f981d

Merged [JDK-8291718](https://bugs.openjdk.org/browse/JDK-8291718) into this as this change is dependent on that patch.
Backed out the linked list enhancement.
This is still a draft, and still need some work on the semantics of some enum statuses, and naming.

Changed the terminology for deoptimization to stop using mark. Because the steps for deoptimization is always the following:
1. Create a list of nmethods to deoptimize (enqueueing)
2. Process the list 
  2.1. Make `nmethod` non entrant
  2.2. (With continuations) patch `nmethod` `NativePostCallNop` instructions
3. Walk all frames and patch return pcs

The mark_for was renamed enqueue, this makes it easier to talk about deoptimization in a context where marking is already a thing (e.g. GCs) and is more correct with regards to what the codes actually does.

All remaining changes are with regards to `deoptimize_done` and that it does not really mean what it says. The comment in `update_recompile_counts()` describes the issue. The most correct thing would be to rename it to something like `deoptimize_post_call_installed` or `deoptimize_post_call_patched` and just leave it like that. No code currently cares if the deoptimization logic is completed (with regards to stack frame walking), only if an `nmethod` has enqueued deoptimization or not, and if `NativePostCallNop` instructions has been installed for loom.

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

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