RFR 8225582: Shenandoah: Enable concurrent evacuation of JNIHandles

[Aleksey Shipilev]

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?

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



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

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

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

Thanks,
-Aleksey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190618/8b9f0811/signature.asc>