RFR: 8139800: Remove OopsInGenClosure

[Stefan Karlsson]

Thanks for reviewing!

StefanK

On 2020-08-27 14:07, stefan.johansson at oracle.com wrote:
> 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