On Mon, 29 Aug 2022 08:35:58 GMT, Axel Boldt-Christmas <abold...@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.
>>
>> _Update_
>> ---
>> Testing after 11d9dd2: Oracle platforms tier 1-5
>
> Axel Boldt-Christmas has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 29 commits:
>
> - Merge remote-tracking branch 'upstream/master' into JDK-8291237
> - Add asserts and comments
> - Merge remote-tracking branch 'upstream/master' into JDK-8291237
> - Add context active assert
> - Cleanup comment
> - Add list optimization
> - Merge remote-tracking branch 'upstream/master' into JDK-8291237
> - Rename deoptimize_done enum value
> - Add missing code from list revert
> - Initial draft new terminology
> - ... and 19 more: https://git.openjdk.org/jdk/compare/512fee1d...c35f5ed6
Just an update and a call for reviews.
[JDK-8290025](https://bugs.openjdk.org/browse/JDK-8290025) / #9741 was merged
in 030ed7e which makes this change easier to reson about as the lifetime of
nmethods are simpler. The merge was tested on oracle platforms tier 1-5.
-------------
PR: https://git.openjdk.org/jdk/pull/9655
