RFR: 8231672: Simplify the reference processing parallelization framework [v5]

[Albert Mingkun Yang]

On Wed, 21 Apr 2021 11:42:04 GMT, Leo Korinth <lkorinth 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.
>
> Leo Korinth has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits:
> 
>  - make RefProcTask a StackObj
>  - Merge branch 'master' into _8231672
>  - using proxies instead of ClosureContexts
>  - remove maybe prefix, change in one place to nullable
>  - Merge branch 'master' into _8231672
>  - Changes after first review
>  - 8231672: Simplify the reference processing parallelization framework

Changes requested by ayang (Committer).

src/hotspot/share/gc/parallel/psCompactionManager.hpp line 42:

> 40: 
> 41: class ParCompactionManager : public CHeapObj<mtGC> {
> 42:   friend class CompactionWithStealingTask;

`CompactionWithStealingTask` and `UpdateAndFillClosure` are dead code.

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

> 222: 
> 223:   {
> 224:     RefProcTotalPhaseTimesTracker tt(RefPhase1, &phase_times, this);

`RefProcTotalPhaseTimesTracker` doesn't really use `this`, right?

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

> 784: 
> 785:   proxy_task.prepare_run_task(task, num_queues(), processing_is_mt() ? RefProcThreadModel::Multi : RefProcThreadModel::Single, marks_oops_alive);
> 786:   if (gang != NULL && processing_is_mt()) {

Since `gang` is only used in thin branch, moving it inside `if` might be better. Besides, the assertion on L779 could be replaced by the code below, IMO.

if (processing_is_mt()) {
  WorkGang* gang = Universe::heap()->safepoint_workers();
  assert(gang != nullptr, "...");
  ...
}

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

> 589: enum class RefProcThreadModel { Multi, Single };
> 590: 
> 591: class RefProcTask : StackObj {

Maybe put some comments here, explaining what `RefProcTask` and `RefProcProxyTask` are, their interaction/relation.

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

> 629:   uint index(uint id) const {
> 630:     return (_tm == RefProcThreadModel::Single) ? 0 : id;
> 631:   }

This method is only used by G1; I wonder if it's possible to not include it in the API of `RefProcProxyTask`.

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

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