RFR: 8259668: Make SubTasksDone use-once [v2]

[Albert Mingkun Yang]

On Sun, 7 Feb 2021 05:49:21 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review
>
> Changes requested by kbarrett (Reviewer).

> I also think this change should be made.

Done; I misunderstood it to be the caller of `all_tasks_completed`.

PS: on revising the doc for `all_tasks_completed`, I realized the intention is to assert all tasks are *claimed*, not *completed* (some subtasks might still be running), so I make the rename for better accuracy.

> src/hotspot/share/gc/shared/workgroup.cpp line 364:
> 
>> 362: #ifdef ASSERT
>> 363: void SubTasksDone::all_tasks_completed_impl(uint skipped[], size_t skipped_size) {
>> 364:   if (Atomic::cmpxchg(&_verification_done, false, true)) {
> 
> This verification done check prevents detection of certain kinds of mistakes.  For example, if the first thread did not claim a skipped task but a later one did, we'll miss that.  Given that this function only does anything in a debug-build, and is usually pretty fast because the number of subtasks is small, I don't think there's a good reason to "optimize" it this way.  (Assuming it even is an optimization, as a CAS operation may be relatively expensive.)

It's not an optimization; it's for avoiding getting duplicate assertion failures. Considering all uses of `SubTasksDone` are based on task-based closure, all threads will run the identical closure. Therefore, it's unlikely that a subtask is skipped in one thread but claimed in another. Revised the comments to make the intention clearer.

> src/hotspot/share/gc/shared/workgroup.hpp line 341:
> 
>> 339:   template<typename T0, typename... Ts,
>> 340:           ENABLE_IF(Conjunction<std::is_same<T0, Ts>...>::value)>
>> 341:   void all_tasks_completed(T0 first_skipped, Ts... more_skipped) {
> 
> I think this overload should be treated as the primary that the documentation applies to, with the no-arg overload following and being commented as being the base case for the variadic function.

Done.

> src/hotspot/share/gc/shared/workgroup.cpp line 391:
> 
>> 389: 
>> 390: bool SubTasksDone::valid() {
>> 391:   return _tasks != NULL;
> 
> This function should can never return false, since _tasks is initialized in the constructor and the value deleted in the destructor.  It should be removed and any callers fixed.

Removed.

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

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