[sh/8u] Root processing related backports

[Zhengyu Gu]

Updated: 
http://cr.openjdk.java.net/~zgu/shenandoah/sh-8u-root-processing/webrev.02/

> 
> Minor nits:
> 
>   *) Double ";;" in shenandoahClosures.inline.hpp here:
> 
>    111   T o = oopDesc::load_heap_oop(p);;
> 
Fixed

>   *) 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");

Right, I did not realize the backport (it came in after initial backport.)

Fixed.

> 
>   *) 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;
> 
> 

Fixed.
>   *) 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 }
> 
Hmm ... screwed up somewhere, removed.

>>> *) 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     */
> 
Ok, I digged into 8u reference processor, and yes, jni weak oops are 
cleaned there.

Although, I could not find JDK-8189360 backport, but JVMTI weak oops no 
longer piggyback to JNIHandles::weak_oops_do() ... which I missed.

So, let's call weak_roots_work() unconditional, has it filter out JNI 
weak oops if reference processing is on.

Thanks,

-Zhengyu