RFR: JDK-8260332: ParallelGC: Cooperative pretouch for oldgen expansion [v2]

[Amit Pawar]

On Sun, 14 Mar 2021 21:31:32 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Amit Pawar has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed build issues for some targets and updated with suggested changes.
>
> src/hotspot/share/gc/shared/pretouchTask.hpp line 47:
> 
>> 45: 
>> 46:   void set_task_status(TaskStatus status) { Atomic::release_store(&_task_status, (size_t)status);  }
>> 47:   void set_task_done()                    { Atomic::release_store(&_task_status, (size_t)Done);    }
> 
> There is a convention that when a setter embodies a release_store that release is included in the name e.g. release_set_task_done().

OK, will change.

> src/hotspot/share/gc/shared/pretouchTask.hpp line 51:
> 
>> 49:   void set_task_notready()                { set_task_status(NotReady); }
>> 50:   bool is_task_ready()                    { return Atomic::load(&_task_status) == Ready; }
>> 51:   bool is_task_done()                     { return Atomic::load(&_task_status) == Done;  }
> 
> Given these fields are set with a release_store I would expect to see them read with a load_acquire to match with it. And named eg. is_task_done_acquire().

OK, will change.

> src/hotspot/share/gc/shared/pretouchTask.cpp line 96:
> 
>> 94:     Atomic::release_store(&_cur_addr, _end_addr);
>> 95:     OrderAccess::storestore();
>> 96:     set_task_done();
> 
> The storestore barrier is not needed as the set_task_done() is a release_store which already has a storestore barrier.

OK.

> src/hotspot/share/gc/shared/pretouchTask.cpp line 56:
> 
>> 54: void PretouchTask::reinitialize(char* start_addr, char* end_addr) {
>> 55:   Atomic::release_store(&_cur_addr, start_addr);
>> 56:   Atomic::release_store(&_end_addr, end_addr);
> 
> Back-to-back release-stores have redundant loadstore semantics. You could just use a storestore() barrier after the first release_store, then use plain Atomic::store for the second field.

OK, will change as per your suggestion.

> src/hotspot/share/gc/parallel/mutableSpace.cpp line 149:
> 
>> 147:         // waiting to expand old-gen will join from PSOldGen::expand_for_allocate
>> 148:         // function for pretouch work.
>> 149:         pretouch_task->set_task_ready();
> 
> The storestore is not needed as it is included in the release_store of set_task_ready().

OK.

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

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