On Mon, 1 Aug 2022 04:58:49 GMT, Axel Boldt-Christmas <d...@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
If the nmethod shouldn't be on the marked list at certain state transitions,
how about adding asserts that check this? Using the same "self looped" trick
that the Sweeper removal PR uses might help with that.
-------------
PR: https://git.openjdk.org/jdk/pull/9655
