RFR 8225582: Shenandoah: Enable concurrent evacuation of JNIHandles

[Zhengyu Gu]

Hi Aleksey,

Thanks for reviewing.

On 6/18/19 4:18 PM, Aleksey Shipilev wrote:
> On 6/17/19 9:15 PM, Zhengyu Gu wrote:
>> Narrowed CR to only evacuating JNIHandles.
>>
>> Updated Webrev: http://cr.openjdk.java.net/~zgu/JDK-8225582/webrev.02/
> 
> I know you have updated patch in works, but here are the things that caught my eye in current patch.
> 
> *) Do we really need separate shenandoahConcurrentRoots.*? I.e. is there an additional value over
> keeping those deciders in ShenandoahHeap, given they are accessing ShenandoahHeap anyway?

I would prefer the separation. ShenandoahConcurrentRoots is an AllStatic 
class, and the decision is not solely based on heap, but some other 
factors, especially when concurrent class unloading comes in.

> 
> *) The local var can probably be inlined, and comment be a little clearer, e.g.:
> 
> Instead of:
> 
> 1076     // Include concurrent roots if current cycle can not process those roots concurrently
> 1077     bool include_concurrent_roots = !ShenandoahConcurrentRoots::should_do_concurrent_roots();
> 1078
> 1079     ShenandoahRootEvacuator rp(workers()->active_workers(), ShenandoahPhaseTimings::init_evac,
> include_concurrent_roots);
> 
> Do:
> 
>     // If concurrent cycle cannot process concurrent roots, force their processing here, at safepoint
>      ShenandoahRootEvacuator rp(workers()->active_workers(), ShenandoahPhaseTimings::init_evac,
>                                 !ShenandoahConcurrentRoots::should_do_concurrent_roots());
> 
> 
Okay.

> 
> *) I think it is more consistent to call it "entry_roots" and "op_roots", without "concurrent":
> 
>   389   // Entry methods to normally concurrent GC operations. These set up logging, monitoring
>   390   // for concurrent operation.
>   391   void entry_reset();
>   392   void entry_mark();
>   393   void entry_preclean();
>   394   void entry_concurrent_roots(); //  <---- ere
>   395   void entry_cleanup();
>   396   void entry_evac();
>   397   void entry_updaterefs();
> 
>   415   void op_reset();
>   416   void op_mark();
>   417   void op_preclean();
>   418   void op_concurrent_roots(); // <--- and here
>   419   void op_cleanup();
>   420   void op_conc_evac();
>   421   void op_stw_evac();
>   422   void op_updaterefs();
>   423   void op_traversal();
> 
Fixed

> *) Can we drop default template argument, and use explicit "false" where needed?
> 
> template <bool CONCURRENT = false>
> class ShenandoahJNIHandleRoots {
> 
> *) Maybe use ShenandoahSharedFlag::try_set() here? This would also give you automatic padding.
> 
> Instead of this:
> 
>    47     if (!_claimed && Atomic::cmpxchg(true, &_claimed, false) == false) {
> 
> Do:
> 
>    47     if (_claimed.try_set()) {
> 
> *) New-line to separate "private" and "public" blocks here:

This is gone. For none concurrent case, should still use parallel version.

http://cr.openjdk.java.net/~zgu/JDK-8225582/webrev.03_inc/


> 
> 1588 class ShenandoahConcurrentRootsEvacUpdateTask : public AbstractGangTask {
> 1589 private:
> 1590   ShenandoahJNIHandleRoots<true /*concurrent*/>         _jni_roots;
> 1591 public:
Fixed.

Updated full webrev: http://cr.openjdk.java.net/~zgu/JDK-8225582/webrev.03/

Reran hotspot_gc_shenandoah (fastdebug and release)

-Zhengyu




> 
> Thanks,
> -Aleksey
>