RFR: 8291237: Encapsulate nmethod Deoptimization logic [v8]
Axel Boldt-Christmas
aboldtch at openjdk.org
Fri Aug 26 08:41:34 UTC 2022
> 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 pull request now contains 28 commits:
- 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
- Make _context_active atomic
- ... and 18 more: https://git.openjdk.org/jdk/compare/054c23f4...11d9dd24
-------------
Changes: https://git.openjdk.org/jdk/pull/9655/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9655&range=07
Stats: 753 lines in 27 files changed: 351 ins; 282 del; 120 mod
Patch: https://git.openjdk.org/jdk/pull/9655.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/9655/head:pull/9655
PR: https://git.openjdk.org/jdk/pull/9655
More information about the hotspot-dev
mailing list