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

[Leo Korinth]

On Tue, 16 Mar 2021 22:16:35 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

I agree. I further prefer not having an assert and instead using a reference. I also fear doing unrelated changes will be rejected. This is no new code of mine, it is added so that nullptr is used consistently after push-back from Thomas.

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

The `ResourceMark`s was used in _some_ of the previous nested closures. See removal of  `G1STWRefProcTaskProxy` and `G1CMRefProcTaskProxy`.

> 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)?

This is not the current name. This was updated to AbstractRefProcClosureContext earlier. I will update to AbstractRefProcOperations if you prefer that. I think your suggestion is better.

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

Yes, will change.

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

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