8252104: parallel heap inspection for ShenandoahHeap #67

linzang · 2020-09-08T03:15:10Z

enable parallel heap inspecton for ShenandoahHeap
- preliminary evaluation:
  Time of jmap histo on (8GB heap with 4GB objects)
  - before: 5.186s
  - after : 1.698s

Progress

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

Issue

JDK-8252104: parallel heap inspection for ShenandoahHeap

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 1fddea7c47aaf25608c85bf3fda5de3b29724803
Zhengyu Gu (@zhengyu123 - Reviewer) ⚠️ Review applies to 313d6cb51e6bfd5af65be7e94994a90ec0ed4d98

Download

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

bridgekeeper · 2020-09-08T03:16:44Z

👋 Welcome back lzang! 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 · 2020-09-08T03:17:29Z

@linzang The following labels will be automatically applied to this pull request: hotspot-gc shenandoah.

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

linzang · 2020-09-08T03:18:53Z

/label add "serviceablility"

openjdk · 2020-09-08T03:19:13Z

@linzang Usage: /label <add|remove> [label[, label, ...]]wherelabel` is an additional classification that should be applied to this PR. These labels are valid:

serviceability
hotspot
sound
hotspot-compiler
kulla
i18n
shenandoah
jdk
javadoc
2d
security
swing
hotspot-runtime
jmx
build
nio
beans
core-libs
compiler
net
hotspot-gc
hotspot-jfr
awt

linzang · 2020-09-08T03:21:14Z

/label add serviceability

openjdk · 2020-09-08T03:21:43Z

@linzang
The serviceability label was successfully added.

mlbridge · 2020-09-08T03:24:21Z

Webrevs

04: Full - Incremental (a1b6a76)
03: Full - Incremental (1fddea7c47aaf25608c85bf3fda5de3b29724803)
02: Full - Incremental (313d6cb51e6bfd5af65be7e94994a90ec0ed4d98)
01: Full - Incremental (6645cc27349013450da84bd08373c1b22bd5bd01)
00: Full (7b400d2c48cdb67d97e4c293593218ddf097573c)

shipilev

Thanks, this looks interesting.

I need more time to digest this. It feels like we can merge together the parallel (new) and serial (old) heap iteration code for better maintainability. I could try to do so later.

linzang · 2020-09-09T03:00:26Z

Thanks @shipilev， This is shenandoah support based on parallel heap inspection enabling in serviceability (tracked with https://bugs.openjdk.java.net/browse/JDK-8215624). hope it is helpful for your review.

linzang · 2020-09-11T09:05:53Z

Hi @shipilev,
I have update a PR that trying to reuse code in serial and parallel heap iteration.
would you like to review again? Thanks
-Lin

shipilev · 2020-09-11T10:54:42Z

Thank you, we'll take a look next week.

zhengyu123 · 2020-09-11T13:54:38Z

Hi Lin,

The patch does not compile with assertion on
@@ -1490,7 +1490,7 @@ private:
cl->do_object(obj);
obj->oop_iterate(&oops);
}

assert(q.is_empty(), "should be empty");

assert(q->is_empty(), "should be empty");

I don't like to export following methods as public. Instead, you can declare ShenandoahParallelObjectIterator as friend class of ShenandoahHeap.
void scan_roots_for_iteration(Stack<oop, mtGC>* oop_stack, ObjectIterateScanRootClosure* oops);
bool prepare_aux_bitmap_for_iteration();
void reclaim_aux_bitmap_for_iteration();
Please rename ObjectIterateParScanClosure to ShenandoahObjectIterateParScanClosure for code convention
There is no point to seed mark roots if prepare_aux_bitmap_for_iteration() failed in ShenandoahParallelObjectIterator constructor.

linzang · 2020-09-12T00:15:06Z

Hi @zhengyu123 ,
Thanks for your comments! I have refined the code and update the PR.
-Lin

zhengyu123 · 2020-09-16T14:06:24Z

Hi @zhengyu123 ,
Thanks for your comments! I have refined the code and update the PR.
-Lin

Looks good to me.
@shipilev may want to take a look.

Thanks.

openjdk · 2020-09-16T14:07:31Z

@linzang This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8252104: parallel heap inspection for ShenandoahHeap

Reviewed-by: shade, zgu

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 148 commits pushed to the master branch:

fdce055: 8253253: Binutils tar ball extension update to gz
388c8f2: 8253348: Remove unimplemented JNIHandles::initialize
bca9e55: 8253167: ARM32 builds fail after JDK-8247910
cc7521c: 8252199: Reimplement support of Type 1 fonts without MappedByteBuffer
3d88d38: 8252070: Some platform-specific BLIT optimizations are not effective
83b0537: 8253291: bug7072653.java still failed "Popup window height ... is wrong"
d27835b: 8249142: java/awt/FontClass/CreateFont/DeleteFont.sh is unstable
1438ce0: 8252188: Crash in OrINode::Ideal(PhaseGVN*, bool)+0x8b9
224a30f: 8252311: AArch64: save two words in itable lookup stub
22f7af7: 8253317: The "com/apple/eawt" is missed in the "othervm.dirs" config option
... and 138 more: https://git.openjdk.java.net/jdk/compare/d0f4366a859dd7ba9243495dadb460e78ad27006...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate fdce055a9b487afa62a0f67861c786d071aea878.

As you do not have Committer status in this projectan existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev, @zhengyu123) 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).

shipilev

Sorry for the delay! I think there is a concurrency bug in using Stack from multiple threads. Plus, there are a bunch of stylistic nits.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

shipilev · 2020-09-18T08:00:24Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

-  if (!_aux_bitmap_region_special && !os::uncommit_memory((char*)_aux_bitmap_region.start(), _aux_bitmap_region.byte_size())) {
-    log_warning(gc)("Could not uncommit native memory for auxiliary marking bitmap for heap iteration");
+    // Reset bitmap
+    _aux_bit_map.clear();


Wait a sec, we have just uncommitted the aux bitmap, so clear() is bound to crash?

No, here the aux_bit_map.clear() is called after commit_memory(), so clear() is safe to reset the bitmap.
the uncommit_memory() was moved to reclaim_aux_bitmap_for_iteration() at line 1354

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp

linzang · 2020-09-21T02:33:26Z

Dear @shipilev,
I have updated the patch, would you like to help review again? Thanks!

-Lin

shipilev

Thanks, this looks very good. I have only a few minor stylistic leftovers. Other than that, it seems ready to integrate.

shipilev · 2020-09-21T05:49:56Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

+  }
+};
+
+ParallelObjectIterator* ShenandoahHeap::parallel_object_iterator(uint num_workers) {


num_workers -> workers

shipilev · 2020-09-21T05:50:06Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp

@@ -549,6 +556,8 @@ class ShenandoahHeap : public CollectedHeap {

  // Used for native heap walkers: heap dumpers, mostly
  void object_iterate(ObjectClosure* cl);
+  // Parallel heap iteration support
+  virtual ParallelObjectIterator* parallel_object_iterator(uint thread_num);


thread_num -> workers

shipilev · 2020-09-21T05:51:42Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp

@@ -524,6 +527,10 @@ class ShenandoahHeap : public CollectedHeap {
  // Prepare and finish concurrent unloading
  void prepare_concurrent_unloading();
  void finish_concurrent_unloading();
+  // Heap iteration support
+  void scan_roots_for_iteration(Stack<oop, mtGC>* oop_stack, ObjectIterateScanRootClosure* oops);


Let's typedef Stack<oop, mtGC> ShenandoahScanRootStack and use it everywhere?

As this Stack is also used for iterating all objects (not only roots), so I took the liberty to rename it to ShenandoahScanObjectStack :)

- enable parallel heap inspecton for ShenandoahHeap - preliminary evaluation: Time of jmap histo on (8GB heap with 4GB objects) * before: 5.186s * after : 1.698s

linzang · 2020-09-21T07:14:43Z

Dear @shipilev,
I just updated the patch. may I ask your help to review and merge it if there is no problem? Thanks!
-Lin

linzang · 2020-09-21T07:15:46Z

/integrate

openjdk · 2020-09-21T07:16:41Z

@linzang
Your change (at version a1b6a76) is now ready to be sponsored by a Committer.

shipilev · 2020-09-21T07:23:59Z

Thank you, this looks good. I'll run a few local test and then sponsor.

shipilev · 2020-09-21T07:50:00Z

/sponsor

openjdk · 2020-09-21T07:51:05Z

@shipilev @linzang Since your change was applied there have been 148 commits pushed to the master branch:

fdce055: 8253253: Binutils tar ball extension update to gz
388c8f2: 8253348: Remove unimplemented JNIHandles::initialize
bca9e55: 8253167: ARM32 builds fail after JDK-8247910
cc7521c: 8252199: Reimplement support of Type 1 fonts without MappedByteBuffer
3d88d38: 8252070: Some platform-specific BLIT optimizations are not effective
83b0537: 8253291: bug7072653.java still failed "Popup window height ... is wrong"
d27835b: 8249142: java/awt/FontClass/CreateFont/DeleteFont.sh is unstable
1438ce0: 8252188: Crash in OrINode::Ideal(PhaseGVN*, bool)+0x8b9
224a30f: 8252311: AArch64: save two words in itable lookup stub
22f7af7: 8253317: The "com/apple/eawt" is missed in the "othervm.dirs" config option
... and 138 more: https://git.openjdk.java.net/jdk/compare/d0f4366a859dd7ba9243495dadb460e78ad27006...master

Your commit was automatically rebased without conflicts.

Pushed as commit 34ec1be.

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

https://github.com/rivosinc/riscv-code-samples/blob/b7582afe988c53e9bd07426ec064fe9907ca1750/zvk-vector-crypto/zvknh.s

openjdk bot added the rfr Pull request is ready for review label Sep 8, 2020

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Sep 8, 2020

openjdk bot added the serviceability serviceability-dev@openjdk.org label Sep 8, 2020

shipilev reviewed Sep 8, 2020

View reviewed changes

linzang force-pushed the s-par branch from 7b400d2 to 6645cc2 Compare September 11, 2020 09:04

linzang force-pushed the s-par branch from 6645cc2 to 313d6cb Compare September 12, 2020 00:12

zhengyu123 approved these changes Sep 16, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 16, 2020

shipilev suggested changes Sep 18, 2020

View reviewed changes

linzang force-pushed the s-par branch from 313d6cb to 1fddea7 Compare September 21, 2020 02:31

shipilev approved these changes Sep 21, 2020

View reviewed changes

linzang force-pushed the s-par branch from 1fddea7 to a1b6a76 Compare September 21, 2020 07:13

8252104: parallel heap inspection for ShenandoahHeap

a1b6a76

- enable parallel heap inspecton for ShenandoahHeap - preliminary evaluation: Time of jmap histo on (8GB heap with 4GB objects) * before: 5.186s * after : 1.698s

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 21, 2020

openjdk bot closed this Sep 21, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 21, 2020

linzang deleted the s-par branch September 21, 2020 07:55

robehn pushed a commit to robehn/jdk that referenced this pull request Aug 15, 2023

Reimplement sha256 and sha512 stubs with latest sample (openjdk#67)

1cd2374

https://github.com/rivosinc/riscv-code-samples/blob/b7582afe988c53e9bd07426ec064fe9907ca1750/zvk-vector-crypto/zvknh.s

caojoshua added a commit to caojoshua/jdk that referenced this pull request Aug 17, 2023

[PEA] Fix typo in run_load_self_into_field.sh (openjdk#67)

a5a6e42

8252104: parallel heap inspection for ShenandoahHeap #67

8252104: parallel heap inspection for ShenandoahHeap #67

linzang commented Sep 8, 2020 •

edited by openjdk bot

bridgekeeper bot commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

linzang commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

linzang commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

mlbridge bot commented Sep 8, 2020 •

edited

shipilev left a comment

linzang commented Sep 9, 2020

linzang commented Sep 11, 2020

shipilev commented Sep 11, 2020

zhengyu123 commented Sep 11, 2020

linzang commented Sep 12, 2020

zhengyu123 commented Sep 16, 2020

openjdk bot commented Sep 16, 2020 •

edited

shipilev left a comment

shipilev Sep 18, 2020

linzang Sep 21, 2020

linzang commented Sep 21, 2020

shipilev left a comment

shipilev Sep 21, 2020

shipilev Sep 21, 2020

shipilev Sep 21, 2020

linzang Sep 21, 2020

linzang commented Sep 21, 2020

linzang commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

shipilev commented Sep 21, 2020

shipilev commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

8252104: parallel heap inspection for ShenandoahHeap #67

8252104: parallel heap inspection for ShenandoahHeap #67

Conversation

linzang commented Sep 8, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

linzang commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

linzang commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

mlbridge bot commented Sep 8, 2020 • edited

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

linzang commented Sep 9, 2020

linzang commented Sep 11, 2020

shipilev commented Sep 11, 2020

zhengyu123 commented Sep 11, 2020

linzang commented Sep 12, 2020

zhengyu123 commented Sep 16, 2020

openjdk bot commented Sep 16, 2020 • edited

shipilev left a comment

Choose a reason for hiding this comment

shipilev Sep 18, 2020

Choose a reason for hiding this comment

linzang Sep 21, 2020

Choose a reason for hiding this comment

linzang commented Sep 21, 2020

shipilev left a comment

Choose a reason for hiding this comment

shipilev Sep 21, 2020

Choose a reason for hiding this comment

shipilev Sep 21, 2020

Choose a reason for hiding this comment

shipilev Sep 21, 2020

Choose a reason for hiding this comment

linzang Sep 21, 2020

Choose a reason for hiding this comment

linzang commented Sep 21, 2020

linzang commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

shipilev commented Sep 21, 2020

shipilev commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

linzang commented Sep 8, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 8, 2020 •

edited

openjdk bot commented Sep 16, 2020 •

edited