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

[Kim Barrett]

On Thu, 4 Feb 2021 15:41:59 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> After JDK-8260574, a instance of `SubTasksDone` is never reused, so part of its APIs could be revised: `clear()` and the code calling it is removed.
>> 
>> With this patch, `all_tasks_completed` contains only assertion. Kim suggested moving this assertion logic to `~SubTasksDone`, but that could defer the assertion violation. For example, in the case of `G1FullGCMarkTask::work`, there is a significant amount of code running btw the instance when all subtasks are claimed (where `all_tasks_completed` is called in this PR) and `~SubTasksDone`. In the interest of having more precise location where bugs may lie, I have kept `all_tasks_completed` in the original place. More comments on this are welcome.
>
> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review

Changes requested by kbarrett (Reviewer).

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

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.

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.

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

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