[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8291237: Encapsulate nmethod Deoptimization logic [v9]
From:       Axel Boldt-Christmas <aboldtch () openjdk ! org>
Date:       2022-08-29 8:35:58
Message-ID: TJYCDjO4tBDPKqokmBEinpaRDkTLZcNfs5sChD9kmg0=.2cd9f6e1-e438-4e97-912e-feef4779b0a3 () github ! com
[Download RAW message or body]

> 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

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

Changes: https://git.openjdk.org/jdk/pull/9655/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9655&range=08
  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


[prev in list] [next in list] [prev in thread] [next in thread]