RFR: 8139800: Remove OopsInGenClosure

[stefan.johansson at oracle.com]

Hi StefanK,

Looks good, and like Kim I really appreciate the very clear explanation :)

Thanks,
StefanJ

On 2020-08-25 12:15, Stefan Karlsson wrote:
> Hi all,
> 
> Please review this patch to remove OopsInGenClosure.
> 
> https://cr.openjdk.java.net/~stefank/8139800/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8139800
> 
> The patch builds upon the cleanup patches that are current out for 
> review: JDK-8252245, JDK-8252289, JDK-8252294.
> 
> - The preparation patches remove most external usages of 
> OopsInGenClosure. However, there are two usages left: 
> oop_since_save_marks_iterate and young_process_roots. These usages call 
> set_generation before applying the closure, and reset_generation() 
> after. I'll explain below why those calls are not needed anymore:
> 
> ---
> 
> After the recent cleanups, FastScanClosure is the only concrete class of 
> OopsInGenClosure. FastScanClosure is used to "scan" the young generation 
> (DefNew). But, it is actually used in two different situations, and 
> require different barrier code depending on where it is used:
> 
> 1) Scan the old gen for pointers into the young gen, and apply a store 
> barrier.
> 2) Scan non-old gen parts, including oops in CLDs (and associated Metadata)
> 
> This can be seen here, where the passed in boolean corresponds to the 
> FastScanClosure::_gc_barrier variable:
> 
>   573   FastScanClosure fsc_with_no_gc_barrier(this, false);
>   574   FastScanClosure fsc_with_gc_barrier(this, true);
> 
> This in turn is used here:
> 
>    77 template <class T> inline void FastScanClosure::do_oop_work(T* p) {
>    ... < Handling the pointer int young gen >
>    87       if (is_scanning_a_cld()) {
>    88         do_cld_barrier();
>    89       } else if (_gc_barrier) {
>    90         // Now call parent closure
>    91         do_barrier(p);
>    92       }
> 
> Where line 91 deals with the (1) case above, and 87-88 deals with the 
> (2) case.
> 
> The do_barrier function called in 91, is provided by OopsInGenClosure. 
> The CLD barrier code is provided by OopsInClassLoaderDataOrGenClosure. 
> This class inherits from OopsInGenClosure, but doesn't use the 
> functionality. It inherits from OopsInGenClosure so that they both can 
> be passed through the same code path.
> 
> In the patch common code is extracted into a revised FastScanClosure 
> super class, that only performs the young gen scanning code, and 
> delegates the barrier code to the derived classes.
> 
> Two derived classes are created:
> - DefNewYoungerGenClosure provides the old-to-young gen write barrier 
> and is used for case (1).
> - DefNewScanClosure provides the CLD barrier for case (2).
> 
> After the separation, it's apparent that DefNewScanClosure doesn't use 
> the OopsInGenClosure code, and the only external usages are calls to 
> set_generation/reset_generation. There are no actual usages of the 
> values set by those functions. The set_generation/reset_generation calls 
> that can be removed are here:
> 
> SerialHeap::young_process_roots:
> ...
>     if (_process_strong_tasks->try_claim_task(GCH_PS_younger_gens)) {
> -    root_closure->reset_generation();
>     }
> 
> and  template <typename OopClosureType>
> 
>   void DefNewGeneration::oop_since_save_marks_iterate(OopClosureType* cl) {
> -  cl->set_generation(this);
>     eden()->oop_since_save_marks_iterate(cl);
>     to()->oop_since_save_marks_iterate(cl);
>     from()->oop_since_save_marks_iterate(cl);
> -  cl->reset_generation();
> 
> 
> DefNewYoungerGenClosure does use the variables changed by 
> set_generation/reset_generation, but the only callers always pass in the 
> old generation:
> 
>   void TenuredGeneration::oop_since_save_marks_iterate(OopClosureType* 
> blk) {
> -  blk->set_generation(this);
>     _the_space->oop_since_save_marks_iterate(blk);
> -  blk->reset_generation();
> 
> 
> and:
> 
> void SerialHeap::young_process_roots
> ...
> -  old_gen_closure->set_generation(_old_gen);
>     rem_set()->at_younger_refs_iterate();
>     old_gen()->younger_refs_iterate(old_gen_closure, scope->n_threads());
> -  old_gen_closure->reset_generation();
> 
> So, instead of doing this over and over again, the code is moved to the 
> constructor. After that, there's no external usages of OopsInGenClosure, 
> and the class can be removed from the class hierarchy.
> 
> ---
> 
> - I've renamed some of the variables to make it more apparent what 
> generation the code is dealing with.
> 
> - Note: The FastScanClosure and its derived classes should be moved to 
> gc/serial, but I've left that for a separate RFE.
> 
> - Note: GCH_PS_younger_gens should probably be removed, but I've left 
> that cleanup for a separate RFE.
> 
> Testing: Testing on tier1-tier7 on Linux with forced usages of 
> -XX:+UseSerialGC.
> 
> Thanks,
> StefanK