RFR: 8231672: Simplify the reference processing parallelization framework

[Leo Korinth]

On Thu, 4 Mar 2021 09:17:23 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> With the change of parallel gc to use the same workgang implementation that other gcs uses, it is possible to remove the abstraction used to hide different worker thread management implementations (TaskExecutor).
>> 
>> At the same time as removing the TaskExecutor, also remove part of the special casing (mostly code duplication), for single threaded execution. 
>> 
>> I consider the new method `prepare_run_task` that modifies the state of the context a step backwards from what was. However, I think removing the Executor with its proxy-tasks and removing separate code paths for serial execution makes the change worth this step back. We also removes ~270 lines of code.
>> 
>> Some comments:
>> 1) 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?
>> 
>> 2) I added an enum class ThreadModel instead of using a boolean, this could also be used in many more places. I dislike sending lots of bools with a comment like `true /* _is_mt */`. It also adds type safety if a method takes many bools. However, with this limited change, and not many hard-coded bools, it feels a bit overkill and I am willing to remove the enum, what do you think?
>> 
>> Testing:
>> hotspot_gc  and tier 1-3 has passed earlier versions before minor cleanups. I intend to re-run tests after review changes.
>
> 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.

Will improve.

> 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.

Okay.

> src/hotspot/share/gc/serial/serialClosureContext.hpp line 30:
> 
>> 28: #include "gc/shared/referenceProcessor.hpp"
>> 29: 
>> 30: class SerialClosureContext : public AbstractClosureContext {
> 
> Maybe "SerialGCRefProcClosureContext"?

Okay.

> src/hotspot/share/gc/serial/serialClosureContext.hpp line 34:
> 
>> 32:   OopClosure& _keep_alive;
>> 33:   VoidClosure& _complete_gc;
>> 34: public:
> 
> newline missing

Okay.

> 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.

Okay (and I agree).

> 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.

Okay.

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

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