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

Kim Barrett kbarrett at openjdk.java.net
Tue Mar 16 22:31:16 UTC 2021


On Thu, 11 Mar 2021 10:18:27 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 incrementally with one additional commit since the last revision:
> 
>   remove maybe prefix, change in one place to nullable

The "Context" suffix isn't suggestive of what it does for me.  "context" suggests scope or surroundings. I think "Group" would be better; as this is a collection of inter-related things. I thought of some others, but many are already overused while others were way to obscure. Another possibility is "xxxClosureContext" => "xxxClosures", though perhaps that pluralization is too subtle.

I think the life-cycle management of the closures by the context objects is a problem. I'm guessing this is to lazily construct the closures? But as used this may be constructing closures over storage containing an already constructed closure, without calling the destructor. This is all UB if the destructors have any relevant side effects (3.8/4). Is that guaranteed for all such closures? I'm not sure I like that requirement on the closure types, and certainly not without some documentation making that explicit. We can't use is_trivial_destructor<> as a safety net since these closures have non-trivial destructors (they are virtual).

Lazy construction and as-needed destruction could be done via having a parallel set of is_constructed flags.  Context destructor cleans up accordingly.

Or maybe have a high-water construction tracker and have prepare_run_task do construction to advance that high-water mark as needed.  Again, Context destructor cleans up accordingly.

In the various context classes, I think the virtual overrides should be explicitly marked "virtual", as that's usually made explicit in our code. (And I've finally sent out JDK-8254050.)

I think there's confusion over determining the number of workers to use.  There are various places that are using active_workers() where total_workers() might be better, for the same reasons as in
https://bugs.openjdk.java.net/browse/JDK-8258985
But I think that same confusion exists in the old code, so I'm okay with treating this as a pre-existing issue for a different change.

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; }

I'd like more spaces around operators and punctuation in that value expression.

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

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

This seems like an overly general name. This is related to reference
processing I think (not going to be used elsewhere), so I think should be
named accordingly.  ClosureContext also seems odd.  Maybe something
like AbstractRefProcOperations?  Handlers (slight bleh)?

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

> 581: 
> 582:   virtual void work(uint worker_id) {
> 583:     ResourceMark rm;

There are a number of places like here where a ResourceMark has been introduced.  I've not been able to figure out what these are for.  And that makes me wonder whether they are in the right place.

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

> 2024: }
> 2025: 
> 2026: void steal_marking_work(TaskTerminator& terminator, uint worker_id) {

If this needs to be made non-file-scoped then it should be moved into some relevant class.

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

> 189: 
> 190:   virtual void do_void() {
> 191:     assert(_promotion_manager != nullptr, "Sanity");

I'd prefer this assert was in the constructor.

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

> 590: class AbstractRefProcClosureContext {
> 591: public:
> 592:   virtual BoolObjectClosure* is_alive(uint worker_id) = 0;

Missing destructor for base class - should be public and virtual or non-public and non-virtual.  As these seem to always be stack allocated, a protected default destructor would be appropriate.

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

> 588: enum class RefProcThreadModel { Multi, Single };
> 589: 
> 590: class AbstractRefProcClosureContext {

I think these are always stack allocated, so derive from StackObj?  (To at least signal intent.)

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

Changes requested by kbarrett (Reviewer).

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



More information about the hotspot-gc-dev mailing list