RFR: 8252104: parallel heap inspection for ShenandoahHeap [v3]
Aleksey Shipilev
shade at openjdk.java.net
Fri Sep 18 08:16:38 UTC 2020
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
More information about the serviceability-dev
mailing list