[sh/8u] Root processing related backports

[Aleksey Shipilev]

On 5/12/20 10:28 PM, Zhengyu Gu wrote:
> Updated: 
> http://cr.openjdk.java.net/~zgu/shenandoah/sh-8u-root-processing/webrev.01/

Looks much better, thanks!

Minor nits:

 *) Double ";;" in shenandoahClosures.inline.hpp here:

  111   T o = oopDesc::load_heap_oop(p);;

 *) In here, we can pull ShenandoahWeakRoots out of both branches?

 768 void ShenandoahConcurrentMark::weak_roots_work() {
 769   ShenandoahHeap* const heap = ShenandoahHeap::heap();
 770   ShenandoahIsAliveSelector is_alive;
 771
 772   if (heap->has_forwarded_objects()) {
 773     ShenandoahWeakUpdateClosure cl;
 774     ShenandoahWeakRoots weak_roots;
 775     weak_roots.weak_oops_do(is_alive.is_alive_closure(), &cl, 0);
 776   } else {
 777     DoNothingClosure cl;
 778     ShenandoahWeakRoots weak_roots;
 779     weak_roots.weak_oops_do(is_alive.is_alive_closure(), &cl, 0);
 780   }
 781 }

 *) This block in op_final_mark() looks redundant:

1457     if (has_forwarded_objects()) {
1458       // Degen may be caused by failed evacuation of roots
1459       if (is_degenerated_gc_in_progress()) {
1460         concurrent_mark()->update_roots(ShenandoahPhaseTimings::degen_gc_update_roots);
1461       } else {
1462         concurrent_mark()->update_thread_roots(ShenandoahPhaseTimings::update_roots);
1463       }
1464     }

 Look, it is removed here:
   https://hg.openjdk.java.net/jdk/jdk/rev/61f6c19d1a56#l3.23

 ...and there is already the assert here:

1446   assert(!has_forwarded_objects(), "No forwarded objects on this path");

 *) I think all other fields are in plural form, so it should be e.g. "_jvmti_roots"?

  51 class ShenandoahSerialRoots {
  52 private:
  53   ShenandoahSerialRoot            _universe_root;
  54   ShenandoahSerialRoot            _management_root;
  55   ShenandoahSerialRoot            _jvmti_root;
  56   ShenandoahSerialRoot            _jni_handle_root;
  57   ShenandoahSerialRoot            _flat_profiler_root;


 *) What are these additional checks for _invalid_phase?

 132 ShenandoahGCWorkerPhase::ShenandoahGCWorkerPhase(const ShenandoahPhaseTimings::Phase phase) :
 133     _timings(ShenandoahHeap::heap()->phase_timings()), _phase(phase) {
 134   if (_phase != ShenandoahPhaseTimings::_invalid_phase) {
 135     _timings->record_workers_start(_phase);
 136   }
 137 }
 138
 139 ShenandoahGCWorkerPhase::~ShenandoahGCWorkerPhase() {
 140   if (_phase != ShenandoahPhaseTimings::_invalid_phase) {
 141     _timings->record_workers_end(_phase);
 142   }
 143 }

>> *) cleanup_jni_refs() is dropped in shenandoahConcurrentMark.cpp here, where is it added back? I
>> looked at weak_roots_work(), is it in ShenandoahWeakRoots now? We have to be very accurate about
>> this, because we broke it already at least once :)
> 
> Yes, jni weak handles are encapsulated in ShenandoahWeakRoots now.
> 
>>
>>   501   // When we're done marking everything, we process weak references.
>>   502   if (_heap->process_references()) {
>>   503     weak_refs_work(full_gc);
>>   504   } else {
>>   505     cleanup_jni_refs();
>>   506   }
>>
> Above logic is obvious fault, because processing weak references has 
> nothing to do with jni weak handles.

Kinda. 8u is special here:
  https://mail.openjdk.java.net/pipermail/shenandoah-dev/2019-December/011193.html

But, I can see that ShenandoahConcurrentMark::weak_roots_work() is now called unconditionally, so it
does the same thing? And there is no explicit call to JvmtiExport::weak_oops_do, which makes us
protected from this mess:

 303     // In JDK 8, this is handled by JNIHandles::weak_oops_do. We cannot enter here, because
 304     // it would walk the JvmtiTagMap twice (which is okay) and possibly by multiple threads
 305     // (which is not okay, because that walk is not thread-safe). In subsequent releases,
 306     // it is handled in a more straightforward manner, see JDK-8189360.
 307     /*
 308     ShenandoahForwardedIsAliveClosure is_alive;
 309     JvmtiExport::weak_oops_do(&is_alive, oops);
 310     */

-- 
Thanks,
-Aleksey