RFR: 8259668: Make SubTasksDone use-once

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8259668: Make SubTasksDone use-once

Albert Mingkun Yang
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.

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

Commit messages:
 - once

Changes: https://git.openjdk.java.net/jdk/pull/2383/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2383&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259668
  Stats: 95 lines in 4 files changed: 31 ins; 49 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2383.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2383/head:pull/2383

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259668: Make SubTasksDone use-once

Thomas Schatzl-4
On Wed, 3 Feb 2021 16:26:33 GMT, Albert Mingkun Yang <[hidden email]> 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.

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/shared/workgroup.hpp line 314:

> 312:
> 313:   void all_tasks_completed_impl(uint skipped[], size_t skipped_size) {
> 314: #ifdef ASSERT

Please keep the definition of the method into the .cpp file. It's too long. You can use the DEBUG_ONLY macro here to not need to define it in non-assert code.

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

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

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

Albert Mingkun Yang
In reply to this post by Albert Mingkun Yang
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/2383/files
  - new: https://git.openjdk.java.net/jdk/pull/2383/files/16cf3cec..90add10d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2383&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2383&range=00-01

  Stats: 61 lines in 2 files changed: 30 ins; 29 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2383.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2383/head:pull/2383

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

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

Albert Mingkun Yang
In reply to this post by Thomas Schatzl-4
On Thu, 4 Feb 2021 15:00:21 GMT, Thomas Schatzl <[hidden email]> wrote:

>> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   review
>
> src/hotspot/share/gc/shared/workgroup.hpp line 314:
>
>> 312:
>> 313:   void all_tasks_completed_impl(uint skipped[], size_t skipped_size) {
>> 314: #ifdef ASSERT
>
> Please keep the definition of the method into the .cpp file. It's too long. You can use the DEBUG_ONLY macro here to not need to define it in non-assert code.

Revised.

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

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl-4
In reply to this post by Albert Mingkun Yang
On Thu, 4 Feb 2021 15:41:59 GMT, Albert Mingkun Yang <[hidden email]> 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

Looks good.

As an alternative to wrapping all (two) calls to `all_tasks_completel_impl` in `DEBUG_ONLY`, it is possible to declare it as `NOT_DEBUG_RETURN` outside of `#ifdef ASSERT` in the header file. The current approach is fine with me too.

An additional (as an extra CR, preexisting) cleanup could be moving more method definitions into the cpp file as they do not seem to be performance critical; but as mentioned, this is fine too.

Finally, could you sync the description of the CR in the JIRA with the PR one that there are no actual instances of reuse of this class any more? This is somewhat misleading as I was searching for actual reuse after reading the CR description (and skipping over the PR description) for a few minutes.

Thanks.

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

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

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

Albert Mingkun Yang
On Fri, 5 Feb 2021 09:34:58 GMT, Thomas Schatzl <[hidden email]> wrote:

> An additional (as an extra CR, preexisting) cleanup could be moving more method definitions into the cpp file as they do not seem to be performance critical; but as mentioned, this is fine too.

I think it's best to not to hide the definitions from the caller. The current approach ensures an optimized build will have zero cost from having the assertion; otherwise, the call site needs a `callq` instruction (without LTO), probably insignificant, but a few extra lines in the header is fine, IMO.

> could you sync the description of the CR in the JIRA ...

Done.

Thank you for the review.

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

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
> On Feb 5, 2021, at 6:10 AM, Albert Mingkun Yang <[hidden email]> wrote:
>
> On Fri, 5 Feb 2021 09:34:58 GMT, Thomas Schatzl <[hidden email]> wrote:
>
>> An additional (as an extra CR, preexisting) cleanup could be moving more method definitions into the cpp file as they do not seem to be performance critical; but as mentioned, this is fine too.
>
> I think it's best to not to hide the definitions from the caller. The current approach ensures an optimized build will have zero cost from having the assertion; otherwise, the call site needs a `callq` instruction (without LTO), probably insignificant, but a few extra lines in the header is fine, IMO.

Thomas’s suggestion was to change the class to have

  DEBUG_ONLY(volatile bool _verification_done = false;)
  void all_tasks_completed_impl(uint skipped[], size_t skipped_size) NOT_DEBUG_RETURN;

and eliminate the DEBUG_ONLY wrappers currently cluttering the calls to all_tasks_completed_impl.

A call in a not-debug build will have a trivial empty-bodied function that will be inlined to nothing.
With that change, I would expect any decent compiler to generate no code for a call to
all_tasks_completed in a product build.  We use this pattern a lot, with exactly that expectation.

I also think this change should be made.

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett-2
In reply to this post by Albert Mingkun Yang
On Thu, 4 Feb 2021 15:41:59 GMT, Albert Mingkun Yang <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259668: Make SubTasksDone use-once [v3]

Albert Mingkun Yang
In reply to this post by Albert Mingkun Yang
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/2383/files
  - new: https://git.openjdk.java.net/jdk/pull/2383/files/90add10d..67c399e1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2383&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2383&range=01-02

  Stats: 32 lines in 4 files changed: 4 ins; 15 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2383.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2383/head:pull/2383

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

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

Albert Mingkun Yang
In reply to this post by Kim Barrett-2
On Sun, 7 Feb 2021 05:49:21 GMT, Kim Barrett <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259668: Make SubTasksDone use-once [v3]

Kim Barrett-2
In reply to this post by Albert Mingkun Yang
On Sun, 7 Feb 2021 09:47:08 GMT, Albert Mingkun Yang <[hidden email]> 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

I would prefer to see all_tasks_claimed eliminated rather than made more
precise at the expense of being more complicated. I think the added
precision doesn't provide enough benefit (vs alternatives like checking once
in the destructor) to pay the API, usage, and implementation cost. But
that's matter of opinion, about which Albert feels to the contrary. I think
this change is an improvement regardless of that question, so won't block on
that basis.

So looks good.

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

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259668: Make SubTasksDone use-once [v3]

Albert Mingkun Yang
On Tue, 16 Feb 2021 06:19:13 GMT, Kim Barrett <[hidden email]> wrote:

>> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   review
>
> I would prefer to see all_tasks_claimed eliminated rather than made more
> precise at the expense of being more complicated. I think the added
> precision doesn't provide enough benefit (vs alternatives like checking once
> in the destructor) to pay the API, usage, and implementation cost. But
> that's matter of opinion, about which Albert feels to the contrary. I think
> this change is an improvement regardless of that question, so won't block on
> that basis.
>
> So looks good.

Thanks for the review.

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

PR: https://git.openjdk.java.net/jdk/pull/2383
Reply | Threaded
Open this post in threaded view
|

Integrated: 8259668: Make SubTasksDone use-once

Albert Mingkun Yang
In reply to this post by Albert Mingkun Yang
On Wed, 3 Feb 2021 16:26:33 GMT, Albert Mingkun Yang <[hidden email]> 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.

This pull request has now been integrated.

Changeset: 3cbd16de
Author:    Albert Mingkun Yang <[hidden email]>
Committer: Thomas Schatzl <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/3cbd16de
Stats:     63 lines in 4 files changed: 11 ins; 39 del; 13 mod

8259668: Make SubTasksDone use-once

Reviewed-by: tschatzl, kbarrett

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

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