RFR: 8231672: Simplify the reference processing parallelization framework

[Thomas Schatzl]

On Mon, 1 Mar 2021 16:14:18 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

> I use references in some places where they can replace pointers. I could go much further here, but I did not
> want to bloat the pull request, maybe later change all closures to be references as well? Should I create 
> enhancements for this?

If necessary, generally do such changes (vs. the reference processing refactoring) in extra CRs. Changing pointers to references seems completely unrelated.

Fwiw, my personal opinion is to not do that at all for code that has lots of connections with existing code as this results in an awkward mixture in these areas between old an new code which only makes things hard to read (and adds another question when interfacing that code, do I need a reference or a pointer now?), but others may have other opinions.
Also it generally hides the information at the call site whether something is actually passed as reference or as value (at least for classes/structs). Of course, as usual there is no hard rule, and I am good if others think it's the way to go.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1477:

> 1475:     G1CollectedHeap& g1h,
> 1476:     G1ConcurrentMark& cm)
> 1477:     : _max_workers(max_workers),

Please adopt a constructor formatting style already present in the file at least. Please do not add new ones.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3359:

> 3357:       return ::new (&_serial_keep_alive) G1CopyingKeepAliveClosure(&_g1h, _pss.state_for_worker(0));
> 3358:     }
> 3359:     return ::new (&_keep_alive[worker_id]) G1CopyingKeepAliveClosure(&_g1h, _pss.state_for_worker(worker_id));

I kind of prefer the symmetric
` if (something) {
      return a;
    } else {
      return b;
    }`
    
style than this "dangling" return as the returns are on the same indentation level. Feel free to ignore in multiple places.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1534:

> 1532:     active_workers = clamp(active_workers, 1u, _max_num_tasks);
> 1533: 
> 1534:     // reference processing context.

Initial capitalization.

src/hotspot/share/gc/g1/g1FullCollector.cpp line 232:

> 230: }
> 231: 
> 232: class G1FullRefProcClosureContext : public AbstractClosureContext {

Please add "GC" after "Full" because this naming begs the question what a "partial" context would look like.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1494:

> 1492:     return ::new (&_keep_alive[index(worker_id, _tm)]) G1CMKeepAliveAndDrainClosure(&_cm, _cm.task(index(worker_id, _tm)), _tm == ThreadModel::Single );
> 1493:   }
> 1494:   VoidClosure* complete_gc(uint worker_id)    {

Inconsistent spacing between methods, sometimes there is a newline, sometimes not. I would prefer the additional newline. This is also the case in the other file.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3364:

> 3362:     assert(worker_id < _queues || _tm == ThreadModel::Single, "sanity");
> 3363:     if (_tm == ThreadModel::Single) {
> 3364:       return ::new (&_serial_complete_gc) G1STWDrainQueueClosure(&_g1h, _pss.state_for_worker(0));

Not sure why this closure needs to be reinitialized to the same values as in the constructor. Just return `&_serial_complete_gc` should be fine. I understand that for the parallel case, as they haven't been initialized yet.

src/hotspot/share/gc/shared/referenceProcessor.hpp line 590:

> 588: enum class ThreadModel {Multi, Single};
> 589: 
> 590: class AbstractClosureContext {

Could the `AbstractClosureContext` be renamed to something like `AbstractRefProcClosureContext` (remove the `Abstract` if too long - it does not really carry a lot of information). The current name is too generic and does not indicate in any way where it belongs to.

src/hotspot/share/gc/shared/referenceProcessor.hpp line 596:

> 594:   virtual VoidClosure* complete_gc(uint worker_id) = 0;
> 595:   virtual void prepare_run_task(uint queue_count, ThreadModel tm, bool marks_oops_alive) = 0;
> 596:   uint index(uint id, ThreadModel tm) { return (tm==ThreadModel::Single)?0:id; }

Please add spaces between operands and operators.

src/hotspot/share/gc/shared/referenceProcessor.hpp line 588:

> 586: };
> 587: 
> 588: enum class ThreadModel {Multi, Single};

I am fine with adding the enum, but the name `ThreadModel`, at least as long as it is only used here, is too generic. Something like `RefProcThreadModel` would be just fine.

Also, please add spaces like this `{ Multi, Single }`

src/hotspot/share/gc/shared/referenceProcessor.cpp line 200:

> 198: 
> 199: ReferenceProcessorStats ReferenceProcessor::process_discovered_references(
> 200:   AbstractClosureContext& closure_context,

Please fix indendation of parameters according to Hotspot style.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 523:

> 521: public:
> 522:   RefProcTask(
> 523:     const char* name,

Indentation, parameter list.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 540:

> 538:                     AbstractClosureContext&       closure_context)
> 539:   : RefProcTask("RefProcPhase1Task",
> 540:             ref_processor,

Indentation.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 800:

> 798:   assert(gang != NULL || !processing_is_mt(), "can not dispatch multi threaded without a work gang");
> 799:   log_debug(gc, ref)("ReferenceProcessor::execute queues: %d, %s, marks_oops_alive: %s",
> 800:     num_queues(), processing_is_mt() ? "ThreadModel::Multi" : "ThreadModel::Single", marks_oops_alive?"true":"false");

Indentation. One parameter per line please if they are long. Spaces between operators. Please also add a newline here so that the "boring" assertions are separated a bit more from the actual code.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 808:

> 806:     gang->run_task(&task, num_queues());
> 807:   } else {
> 808:     log_debug(gc, ref)("Serial loop: %d\n", _max_num_queues);

This log message seems to be a debugging message, not offering a lot information. Please remove.

src/hotspot/share/gc/serial/serialClosureContext.hpp line 30:

> 28: #include "gc/shared/referenceProcessor.hpp"
> 29: 
> 30: class SerialClosureContext : public AbstractClosureContext {

Maybe "SerialGCRefProcClosureContext"?

src/hotspot/share/gc/serial/serialClosureContext.hpp line 34:

> 32:   OopClosure& _keep_alive;
> 33:   VoidClosure& _complete_gc;
> 34: public:

newline missing

src/hotspot/share/gc/serial/serialClosureContext.hpp line 40:

> 38:   OopClosure* keep_alive(uint worker_id)                                         { return &_keep_alive; }
> 39:   VoidClosure* complete_gc(uint worker_id)                                       { return &_complete_gc; }
> 40:   void prepare_run_task(uint queue_count, ThreadModel tm, bool marks_oops_alive) { log_debug(gc, ref)("SerialClosureContext: prepare_run_task"); };

Please add more spacing. Also I'm not a fan of excessive alignment of methods.

src/hotspot/share/gc/parallel/psScavenge.cpp line 196:

> 194:               "stacks should be empty at this point");
> 195: 
> 196:     if (_maybe_terminator != nullptr) {

Please change the `!= NULL` to `!= nullptr` at least in the same method too (above). It is not required to fix it everywhere (in this file) for this change, but using different styles in the same method is weird.

src/hotspot/share/gc/parallel/psScavenge.cpp line 183:

> 181:  private:
> 182:   PSPromotionManager* _promotion_manager;
> 183:   TaskTerminator* _maybe_terminator;

What does the `_maybe` in `_maybe_terminator` indicate? Just naming it `_terminator` seems as fine as a `nullptr` seems like a valid value.
It is explained in `PSRefProcClosureContext`, but it seems too much to add this everywhere where a pointer could be a valid `nullptr`.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 2098:

> 2096: };
> 2097: 
> 2098: class PCRefProcClosureContext : public AbstractClosureContext {

The abbreviation used for Parallel Compact is `ParallelCompact` or `ParCompact[ion]` or `PSParallelCompact` otherwise afaics. Please do not add even more of those (yeah, there is already `PCAddThreadRootsMarkingTaskClosure`)...

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 2125:

> 2123:     return ::new (&_keep_alive[worker_id]) PCMarkAndPushClosure(cm);
> 2124:   }
> 2125:   VoidClosure* complete_gc(uint worker_id) {

Newline between method definitions.

src/hotspot/share/gc/g1/g1FullCollector.cpp line 246:

> 244:     FREE_C_HEAP_ARRAY(G1FullKeepAliveClosure, _keep_alive);
> 245:   }
> 246:   BoolObjectClosure* is_alive(uint worker_id) {

Newlines between definitions.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3317:

> 3315: class G1STWRefProcClosureContext : public AbstractClosureContext {
> 3316:   uint                           _max_workers;
> 3317:   uint                           _queues;

Please avoid lining up member names. That is hard to read. Group them by functionality/contents.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3331:

> 3329: public:
> 3330:   G1STWRefProcClosureContext(
> 3331:     uint max_workers,

Parameter indentation.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3354:

> 3352: 
> 3353:   BoolObjectClosure* is_alive(uint worker_id) { return &_is_alive; }
> 3354:   OopClosure* keep_alive(uint worker_id) {

Newline between definitions.

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

Changes requested by tschatzl (Reviewer).

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