RFR: 8252104: parallel heap inspection for ShenandoahHeap [v3]

[Aleksey Shipilev]

On Sat, 12 Sep 2020 00:17:58 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> - enable parallel heap inspecton for ShenandoahHeap
>>   - preliminary evaluation:
>>     Time of jmap histo on (8GB heap with 4GB objects)
>>     * before: 5.186s
>>     * after : 1.698s
>
> Lin Zang has refreshed the contents of this pull request, and previous commits have been removed. The incremental views
> will show differences compared to the previous content of the PR. The pull request contains one new commit since the
> last revision:
>   8252104: parallel heap inspection for ShenandoahHeap
>   
>     - enable parallel heap inspecton for ShenandoahHeap
>     - preliminary evaluation:
>       Time of jmap histo on (8GB heap with 4GB objects)
>       * before: 5.186s
>       * after : 1.698s

Sorry for the delay! I think there is a concurrency bug in using `Stack` from multiple threads. Plus, there are a bunch
of stylistic nits.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1314:

> 1312:
> 1313:   Stack<oop,mtGC> oop_stack;
> 1314:   // root marking

This needs to be close to `scan_roots_for_iteration` call, and the comment should match the style: "// Seed the stack
with root scan".

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1333:

> 1331:
> 1332: bool ShenandoahHeap::prepare_aux_bitmap_for_iteration() {
> 1333:     assert(SafepointSynchronize::is_at_safepoint(), "safe iteration is only available during safepoints");

Indenting should be 2 spaces in this method.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1346:

> 1344:
> 1345: void ShenandoahHeap::scan_roots_for_iteration(Stack<oop, mtGC>* oop_stack, ObjectIterateScanRootClosure* oops) {
> 1346:     // Process GC roots according to current GC cycle.

Indenting should be 2 spaces in this method.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1341:

> 1339:
> 1340:     // Reset bitmap
> 1341:     _aux_bit_map.clear();

Wait a sec, we have just uncommitted the aux bitmap, so `clear()` is bound to crash?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1380:

> 1378:
> 1379:       assert(oopDesc::is_oop(obj), "must be a valid oop");
> 1380:       // parallel mark

No need for this comment, it is obvious from the context.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1470:

> 1468:     for (uint j = 0; j < roots_num; j++) {
> 1469:       uint stack_id = j % _num_workers;
> 1470:       oop obj = _roots_stack.pop();

I am not sure we can `pop()` the stack from multiple threads without any sort of synchronization. It might be better to
just walk the same roots from multiple threads, and then let the bitmap help us figure out if we have visited the root
already. See how `ShenandoahVerifierReachableTask` does it?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1422:

> 1420:     // Procssing roots
> 1421:     _heap->scan_roots_for_iteration(&_roots_stack, &oops);
> 1422:     // prepare worker stacks

All three comments look excessive to me? Also, typo in "Procssing". I'd say:

    _init_ready = _heap->prepare_aux_bitmap_for_iteration();
    if (!_init_ready) {
      return;
    }

    ObjectIterateScanRootClosure oops(_aux_bit_map, &_roots_stack);
    _heap->scan_roots_for_iteration(&_roots_stack, &oops);

    _init_ready = prepare_worker_queues();

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1405:

> 1403:   MarkBitMap*                  _aux_bit_map;
> 1404:   ShenandoahHeap*              _heap;
> 1405:   Stack<oop, mtGC>             _roots_stack; // global roots stack

I don't think we can share the stack here. See the comment near `pop()` below.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1450:

> 1448:
> 1449: private:
> 1450:   // divide global root_stack into worker queues.

Capitalize: "// Divide"

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1453:

> 1451:   bool prepare_worker_queues() {
> 1452:     _task_queues = new ShenandoahObjToScanQueueSet((int) _num_workers);
> 1453:     // initialize queue for every workers

`// Initialize queue for every worker` -- singular.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1465:

> 1463:       return false;
> 1464:     }
> 1465:     // assume that object referencing distribution is related with

This comment is better to merge with the previous one. So that we have:

    // Distribute roots among the workers. Assume that object referencing distribution
    // is related with/ root kind, use round-robin to make every worker have same chance
    // to process every kind of roots
    size_t roots_num = _roots_stack.size();
    if (roots_num == 0) {
      // no work to do.
      return false;
    }

    for (uint j = 0; j < roots_num; j++) {
      uint stack_id = j % _num_workers;
      oop obj = _roots_stack.pop();
      ...

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1484:

> 1482:     ShenandoahObjToScanQueue* q = queue_set->queue(worker_id);
> 1483:     assert(q != NULL, "object iterate queue must not be NULL");
> 1484:     // Task in queue

Unnecessary comment, just have a new line here.

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 41:

> 39: #include "utilities/globalDefinitions.hpp"
> 40: #include "utilities/stack.hpp"
> 41:

Excess new line

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 560:

> 558:   // Used for native heap walkers: heap dumpers, mostly
> 559:   void object_iterate(ObjectClosure* cl);
> 560:   // parallel heap iteration support

Capitalize `// Parallel`

-------------

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/67