> 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/f4067fb3...9f6f981d

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/9655/files
  - new: https://git.openjdk.org/jdk/pull/9655/files/5d26ab8d..9f6f981d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=9655&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9655&range=01-02

  Stats: 15374 lines in 744 files changed: 8974 ins; 4345 del; 2055 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

Reply via email to