8074101: Add verification that all tasks are actually claimed during roots processing #2046

albertnetymk · 2021-01-12T11:13:29Z

The first commit removes some obsolete enum items, while the second commit adds the verification logic. Commit 2 introduces some "empty" task claims for the verification logic, explicitly marked in the comments.

Test: hotspot_gc

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8074101: Add verification that all tasks are actually claimed during roots processing

Reviewers

Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 4d14781
Thomas Schatzl (@tschatzl - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2046/head:pull/2046
$ git checkout pull/2046

bridgekeeper · 2021-01-12T11:14:16Z

👋 Welcome back ayang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-01-12T11:15:16Z

@albertnetymk The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

albertnetymk · 2021-01-12T11:16:10Z

/label hotspot-gc

openjdk · 2021-01-12T11:16:34Z

@albertnetymk
The hotspot-gc label was successfully added.

mlbridge · 2021-01-12T11:20:06Z

Webrevs

04: Full - Incremental (15892bd)
03: Full - Incremental (4d14781)
02: Full - Incremental (aa2d853)
01: Full - Incremental (938dce1dabdb49c2f8e1fd2125d216695295f23c)
00: Full (4fc22b4)

kimbarrett · 2021-01-12T15:02:27Z

src/hotspot/share/gc/g1/g1RootProcessor.cpp

+  // only for verification purpose
+  // already processed in java roots.
+  _process_strong_tasks.try_claim_task(G1RP_PS_CodeCache_oops_do);
+#endif


Rather than these fake claims, consider something like this:

template<typename... Ts> void all_tasks_completed(uint nworkers, Ts... tags) { // Type-check more_skipped are all of the same type as first_skipped. T0 typed_skipped[] = { first_skipped, more_skipped... }; uint skipped[] = { static_cast<uint>(tags)... }; all_tasks_completed_impl(nworkers, skipped, ARRAY_SIZE(skipped)); } void all_tasks_completed(uint nworkers) { all_tasks_completed_impl(nworkers, nullptr, 0); }

Usage:

all_tasks_completed(n_workers(), G1RP_PS_CodeCache_oops_do, G1RP_PS_refProcessor_oops_do)

all_tasks_completed_impl can check that all tasks have been claimed except
the skipped ones, which have not been claimed.

There might be better ways to write the variadic all_tasks_completed. It's
been a while since I've done anything with variadic templates.

Here's a better version of the variadic all_tasks_completed

template<typename T0, typename... Ts, ENABLE_IF(Conjunction<std::is_same<T0, Ts>...>::value)> void all_tasks_completed(uint n_threads, T0 first_skipped, Ts... more_skipped) { static_assert(std::is_convertible<T0, uint>::value, "not convertible"); uint skipped[] = { static_cast<uint>(first_skipped), static_cast<uint>(more_skipped)... }; all_tasks_completed_impl(n_threads, skipped, ARRAY_SIZE(skipped)); }

Conjunction is in metaprogramming/logical.hpp.

Thank you; updated as suggested.

kimbarrett · 2021-01-13T08:30:20Z

src/hotspot/share/gc/shared/workgroup.cpp

+        }
+      }
+      assert(is_skipped, "%d not claimed.", i);
+    }


Can also do a separate loop over skipped to verify _tasks[skipped[i]] == 0 (with appropriate bounds checking).

kimbarrett · 2021-01-13T08:33:24Z

src/hotspot/share/gc/shared/workgroup.hpp

@@ -329,7 +333,18 @@ class SubTasksDone: public CHeapObj<mtInternal> {
  // cleared.)
  //
  // n_threads - Number of threads executing the sub-tasks.
-  void all_tasks_completed(uint n_threads);
+  // followed by vararg skipped tasks


The description comment should be augmented to describe the optional skipped values.

kimbarrett · 2021-01-13T08:35:36Z

src/hotspot/share/gc/g1/g1RootProcessor.cpp

+  // CodeCache is already processed in java roots
+  // refProcessor is not needed since we are inside a safe point
+  _process_strong_tasks.all_tasks_completed(n_workers(),
+      G1RP_PS_CodeCache_oops_do, G1RP_PS_refProcessor_oops_do);


When parameters or arguments are on multiple lines, we usually align all the arguments, and usually one per line. There are some other similar cases elsewhere in this change that I didn't redundantly comment.

kimbarrett · 2021-01-13T08:37:24Z

src/hotspot/share/gc/shared/workgroup.hpp

@@ -26,6 +26,7 @@
 #define SHARE_GC_SHARED_WORKGROUP_HPP

 #include "memory/allocation.hpp"
+#include "metaprogramming/logical.hpp"


Should also add metaprogramming/enableIf.hpp, to avoid implicit include dependency.

Should I include both enableIf.hpp and logical.hpp, or only enableIf.hpp since it includes logical.hpp already?

We don't have any tooling support, but there has been a trend away from relying on implicit includes, because they lead to breakages far away from refactorings that change includes.

Got it; including both then.

albertnetymk · 2021-01-13T10:16:40Z

Addressed all suggestions in the revision.

kimbarrett · 2021-01-13T10:26:22Z

src/hotspot/share/gc/shared/workgroup.hpp

-  // must execute this.  (When the last thread does so, the task array is
-  // cleared.)
+  // The calling thread asserts that it has attempted to claim all the tasks
+  // that it will try to claim.  Tasks that is meant to be skipped must be


s/is meant/are meant/

kimbarrett · 2021-01-13T10:31:13Z

src/hotspot/share/gc/shared/workgroup.hpp

-  // cleared.)
+  // The calling thread asserts that it has attempted to claim all the tasks
+  // that it will try to claim.  Tasks that is meant to be skipped must be
+  // explicitly passed as extra arguments using the variadic version below.


I would drop "using the variadic version below". I think of this as a function that takes some optional "these are expected to be skipped" task designators, and the non-variadic overload is just an implementation detail to handle the base case of there being none of those.

openjdk · 2021-01-13T10:32:43Z

@albertnetymk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8074101: Add verification that all tasks are actually claimed during roots processing

Reviewed-by: kbarrett, tschatzl

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 101 new commits pushed to the master branch:

917f7e9: 8259650: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
3f19ef6: 8202880: Test javax/swing/JPopupMenu/8075063/ContextMenuScrollTest.java fails
68cf65d: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
5dc5d94: 8256110: Create implementation for NSAccessibilityStepper protocol
5f2e280: 8259865: (fs) test/jdk/java/nio/file/attribute/UserDefinedFileAttributeView/Basic.java failing on macOS 10.13
da4cf05: 8258755: jpackage: Invalid 32-bit exe when building app-image
c3bdbf9: 8259238: Clean up Log.java and remove usage of non-final static variables.
6d6a23e: 8259062: Remove MacAppStoreBundler
afd3f78: 8030048: (fs) Support UserDefinedFileAttributeView/extended attributes on OS X / HFS+
bbb93ca: 8256126: Create implementation for NSAccessibilityImage protocol peer
... and 91 more: https://git.openjdk.java.net/jdk/compare/a6ab9e4740024302fa74c3ede286c3fb6776b438...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kimbarrett, @tschatzl) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

albertnetymk · 2021-01-13T11:16:25Z

Addressed suggestions on the comments.

tschatzl

Lgmt.

tschatzl · 2021-01-15T14:37:45Z

src/hotspot/share/gc/shared/workgroup.cpp

-    Atomic::inc(&_claimed);
+  // all non-skipped tasks are claimed
+  for (uint i = 0; i < _n_tasks; ++i) {
+    if (_tasks[i] == 0) {


pre-existing: This could be fixed in a separate CR: _tasks could be an array of bool instead of (u)int. Using an int is a historic artifact of not having a good Atomics library.

Created a ticket (JDK-8259851) for it; will start working on that after this is merged. Thanks for the suggestion.

albertnetymk · 2021-01-18T08:29:19Z

Thanks for the review.

/integrate

albertnetymk · 2021-01-18T08:29:51Z

/integrate

openjdk · 2021-01-18T08:30:01Z

@albertnetymk
Your change (at version 15892bd) is now ready to be sponsored by a Committer.

openjdk · 2021-01-18T08:30:58Z

@albertnetymk
Your change (at version 15892bd) is now ready to be sponsored by a Committer.

tschatzl · 2021-01-18T08:32:54Z

/sponsor

openjdk · 2021-01-18T08:33:48Z

@tschatzl @albertnetymk Since your change was applied there have been 101 commits pushed to the master branch:

917f7e9: 8259650: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
3f19ef6: 8202880: Test javax/swing/JPopupMenu/8075063/ContextMenuScrollTest.java fails
68cf65d: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
5dc5d94: 8256110: Create implementation for NSAccessibilityStepper protocol
5f2e280: 8259865: (fs) test/jdk/java/nio/file/attribute/UserDefinedFileAttributeView/Basic.java failing on macOS 10.13
da4cf05: 8258755: jpackage: Invalid 32-bit exe when building app-image
c3bdbf9: 8259238: Clean up Log.java and remove usage of non-final static variables.
6d6a23e: 8259062: Remove MacAppStoreBundler
afd3f78: 8030048: (fs) Support UserDefinedFileAttributeView/extended attributes on OS X / HFS+
bbb93ca: 8256126: Create implementation for NSAccessibilityImage protocol peer
... and 91 more: https://git.openjdk.java.net/jdk/compare/a6ab9e4740024302fa74c3ede286c3fb6776b438...master

Your commit was automatically rebased without conflicts.

Pushed as commit e93f08e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

albertnetymk added 2 commits January 12, 2021 11:10

remove unused enum items

67aae07

add claim verification

4fc22b4

openjdk bot added the rfr label Jan 12, 2021

openjdk bot added the hotspot label Jan 12, 2021

openjdk bot added the hotspot-gc label Jan 12, 2021

kimbarrett reviewed Jan 12, 2021

View reviewed changes

review

aa2d853

albertnetymk force-pushed the claim branch from 938dce1 to aa2d853 Compare January 12, 2021 22:05

kimbarrett suggested changes Jan 13, 2021

View reviewed changes

review

4d14781

kimbarrett approved these changes Jan 13, 2021

View reviewed changes

openjdk bot added the ready label Jan 13, 2021

review

15892bd

tschatzl approved these changes Jan 15, 2021

View reviewed changes

openjdk bot added the sponsor label Jan 18, 2021

openjdk bot closed this Jan 18, 2021

openjdk bot added the integrated label Jan 18, 2021

openjdk bot removed sponsor ready rfr labels Jan 18, 2021

albertnetymk deleted the claim branch January 18, 2021 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8074101: Add verification that all tasks are actually claimed during roots processing #2046

8074101: Add verification that all tasks are actually claimed during roots processing #2046

albertnetymk commented Jan 12, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 12, 2021

openjdk bot commented Jan 12, 2021

albertnetymk commented Jan 12, 2021

openjdk bot commented Jan 12, 2021

mlbridge bot commented Jan 12, 2021 •

edited

Loading

kimbarrett Jan 12, 2021

kimbarrett Jan 12, 2021

albertnetymk Jan 12, 2021

kimbarrett Jan 13, 2021

kimbarrett Jan 13, 2021

kimbarrett Jan 13, 2021

kimbarrett Jan 13, 2021

albertnetymk Jan 13, 2021

kimbarrett Jan 13, 2021

albertnetymk Jan 13, 2021

albertnetymk commented Jan 13, 2021

kimbarrett Jan 13, 2021

kimbarrett Jan 13, 2021

openjdk bot commented Jan 13, 2021 •

edited

Loading

albertnetymk commented Jan 13, 2021

tschatzl left a comment

tschatzl Jan 15, 2021 •

edited

Loading

albertnetymk Jan 15, 2021

albertnetymk commented Jan 18, 2021

albertnetymk commented Jan 18, 2021

openjdk bot commented Jan 18, 2021

openjdk bot commented Jan 18, 2021

tschatzl commented Jan 18, 2021

openjdk bot commented Jan 18, 2021

8074101: Add verification that all tasks are actually claimed during roots processing #2046

8074101: Add verification that all tasks are actually claimed during roots processing #2046

Conversation

albertnetymk commented Jan 12, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 12, 2021

openjdk bot commented Jan 12, 2021

albertnetymk commented Jan 12, 2021

openjdk bot commented Jan 12, 2021

mlbridge bot commented Jan 12, 2021 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertnetymk commented Jan 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 13, 2021 • edited Loading

albertnetymk commented Jan 13, 2021

tschatzl left a comment

Choose a reason for hiding this comment

tschatzl Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertnetymk commented Jan 18, 2021

albertnetymk commented Jan 18, 2021

openjdk bot commented Jan 18, 2021

openjdk bot commented Jan 18, 2021

tschatzl commented Jan 18, 2021

openjdk bot commented Jan 18, 2021

albertnetymk commented Jan 12, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 12, 2021 •

edited

Loading

openjdk bot commented Jan 13, 2021 •

edited

Loading

tschatzl Jan 15, 2021 •

edited

Loading