8267185: Add string deduplication support to ParallelGC #5085

walulyai · 2021-08-11T13:56:09Z

Hi all,

Please review this change to add string deduplication support to ParallelGC.

Testing: All string deduplication tests, and Tier 1-5.

Progress

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

Issue

JDK-8267185: Add string deduplication support to ParallelGC

Reviewers

Kim Barrett (@kimbarrett - Reviewer)
Albert Mingkun Yang (@albertnetymk - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5085/head:pull/5085
$ git checkout pull/5085

Update a local copy of the PR:
$ git checkout pull/5085
$ git pull https://git.openjdk.java.net/jdk pull/5085/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5085

View PR using the GUI difftool:
$ git pr show -t 5085

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5085.diff

bridgekeeper · 2021-08-11T13:57:13Z

👋 Welcome back iwalulya! 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-08-11T14:00:29Z

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

hotspot-gc

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.

mlbridge · 2021-08-11T14:04:47Z

Webrevs

kimbarrett · 2021-08-11T15:11:57Z

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp

@@ -139,6 +139,9 @@ class ParallelScavengeHeap : public CollectedHeap {
  // Returns JNI_OK on success
  virtual jint initialize();

+  void safepoint_synchronize_begin();
+  void safepoint_synchronize_end();


I see no callers of these new functions.

Oh, never mind. These are overriding virtual function declarations. Please add virtual. The modern style would be to use override, but gcc warns if some but not all of a class's overridden virtual functions use override, and you probably don't want to do that update for this class as part of this change.

kimbarrett · 2021-08-16T07:32:48Z

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp

@@ -284,6 +285,12 @@ inline oop PSPromotionManager::copy_unmarked_to_survivor_space(oop o,
    } else {
      // we'll just push its contents
      push_contents(new_obj);
+
+      if (psStringDedup::is_candidate_from_evacuation(o->klass(), new_obj->age(), new_obj_is_tenured)) {


I think is_candidate_from_evacuation should just take new_obj and new_obj_is_tenured as arguments, and get the klass and age there-in as needed. Maybe there's enough inlining and such that the compiler can optimize away unneeded accesses, but why not make the compiler's and the reader's job easier?

kimbarrett · 2021-08-16T07:35:46Z

src/hotspot/share/gc/parallel/psStringDedup.hpp

+  static bool is_candidate_from_evacuation(const Klass* klass,
+                                           uint age,
+                                           bool obj_is_tenured) {
+    return StringDedup::is_enabled_string(klass) &&


It is better here to first check StringDedup::is_enabled and only get the klass if it's actually needed. G1 uses StringDedup::is_enabled_string because it happens to already have the klass in hand. That's not the case here; but the caller is getting the klass just for calling this function.

kimbarrett · 2021-08-16T07:43:51Z

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp

@@ -284,6 +285,12 @@ inline oop PSPromotionManager::copy_unmarked_to_survivor_space(oop o,
    } else {
      // we'll just push its contents
      push_contents(new_obj);
+
+      if (psStringDedup::is_candidate_from_evacuation(o->klass(), new_obj->age(), new_obj_is_tenured)) {
+        if (!java_lang_String::test_and_set_deduplication_requested(o)) {


Setting the flag on the from-object isn't useful; it needs to be on the to-object. But I don't think this is needed anyway. is_candidate_from_evacuation shouldn't generate duplicate requests.

kimbarrett · 2021-08-16T07:52:33Z

src/hotspot/share/gc/shared/stringdedup/stringDedupConfig.cpp

@@ -116,7 +116,7 @@ size_t StringDedup::Config::desired_table_size(size_t entry_count) {
 bool StringDedup::Config::ergo_initialize() {
  if (!UseStringDeduplication) {
    return true;
-  } else if (!UseG1GC && !UseShenandoahGC && !UseZGC) {
+  } else if (UseEpsilonGC || UseSerialGC) {


This change is a small land mine for the next new GC project. I'd prefer adding !UseParallelGC. Or rewrite to have an else if (UseXXXGC || ...) { ... } else { // not supported } structure.

kimbarrett · 2021-08-16T08:41:10Z

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp

@@ -139,6 +139,9 @@ class ParallelScavengeHeap : public CollectedHeap {
  // Returns JNI_OK on success
  virtual jint initialize();

+  void safepoint_synchronize_begin();
+  void safepoint_synchronize_end();


Oh, never mind. These are overriding virtual function declarations. Please add virtual. The modern style would be to use override, but gcc warns if some but not all of a class's overridden virtual functions use override, and you probably don't want to do that update for this class as part of this change.

kimbarrett

Looks good.

kimbarrett · 2021-08-16T11:10:12Z

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp

@@ -286,10 +286,8 @@ inline oop PSPromotionManager::copy_unmarked_to_survivor_space(oop o,
      // we'll just push its contents
      push_contents(new_obj);

-      if (psStringDedup::is_candidate_from_evacuation(o->klass(), new_obj->age(), new_obj_is_tenured)) {
-        if (!java_lang_String::test_and_set_deduplication_requested(o)) {
+      if (psStringDedup::is_candidate_from_evacuation(new_obj, new_obj_is_tenured)) {
          _string_dedup_requests.add(o);


Update indentation? Or is github not showing the whitespace-only change?

Need to update the indentation! Thanks

openjdk · 2021-08-16T11:12:50Z

@walulyai 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:

8267185: Add string deduplication support to ParallelGC

Reviewed-by: kbarrett, ayang

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 84 new commits pushed to the master branch:

92bde67: 8271946: Cleanup leftovers in Space and subclasses
db9834f: 8258951: java/net/httpclient/HandshakeFailureTest.java failed with "RuntimeException: Not found expected SSLHandshakeException in java.io.IOException"
a81e5e9: 8272654: Mark word accesses should not use Access API
4bd37c3: 8272708: [Test]: Cleanup: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java no longer needs ocspEnabled
ddcd851: 8272602: [macos] not all KEY_PRESSED events sent when control modifier is used
d007be0: 8272700: [macos] Build failure with Xcode 13.0 after JDK-8264848
f4be211: 8270041: Consolidate oopDesc::cas_forward_to() and oopDesc::forward_to_atomic()
b40e8f0: 8271951: Consolidate preserved marks overflow stack in SerialGC
7eccbd4: 8266519: Cleanup resolve() leftovers from BarrierSet et al
9569159: 8272674: Logging missing keytab file in Krb5LoginModule
... and 74 more: https://git.openjdk.java.net/jdk/compare/cd1751c34e974683f3d2734c8ad5823a6ea27295...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

albertnetymk

The rest look fine.

albertnetymk · 2021-08-19T13:44:23Z

src/hotspot/share/gc/parallel/psStringDedup.cpp

+  // Candidate if string is being evacuated from young to old but has not
+  // reached the deduplication age threshold, i.e. has not previously been a
+  // candidate during its life in the young generation.
+  return PSParallelCompact::space_id(cast_from_oop<HeapWord*>(java_string)) > PSParallelCompact::old_space_id &&


I wonder if the space-id comparison can be replaced with sth like PSScavenge::is_obj_in_young(java_string); we are checking if this java_string live in young-gen, right?

kimbarrett · 2021-08-20T00:02:33Z

src/hotspot/share/gc/parallel/psStringDedup.hpp

+#ifndef SHARE_GC_PARALLEL_PSSTRINGDEDUP_HPP
+#define SHARE_GC_PARALLEL_PSSTRINGDEDUP_HPP
+
+#include "classfile/javaClasses.inline.hpp"


".inline.hpp" files must not be included by ".hpp" files.

kimbarrett · 2021-08-20T06:05:34Z

src/hotspot/share/gc/parallel/psStringDedup.hpp

+  // Candidate selection policy for young during evacuation.
+  // If to is young then age should be the new (survivor's) age.
+  // if to is old then age should be the age of the copied from object.
+  static bool is_candidate_from_evacuation(oop obj,


This comment was just copied from G1 and isn't right for this implementation.

Leftover from an earlier version, fixed.

kimbarrett

Looks good.

walulyai · 2021-08-20T11:28:52Z

Thanks @albertnetymk and @kimbarrett for the reviews!

/integrate

openjdk · 2021-08-20T11:30:10Z

Going to push as commit fb1dfc6.
Since your change was applied there have been 85 commits pushed to the master branch:

d874e96: 8271579: G1: Move copy before CAS in do_copy_to_survivor_space
92bde67: 8271946: Cleanup leftovers in Space and subclasses
db9834f: 8258951: java/net/httpclient/HandshakeFailureTest.java failed with "RuntimeException: Not found expected SSLHandshakeException in java.io.IOException"
a81e5e9: 8272654: Mark word accesses should not use Access API
4bd37c3: 8272708: [Test]: Cleanup: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java no longer needs ocspEnabled
ddcd851: 8272602: [macos] not all KEY_PRESSED events sent when control modifier is used
d007be0: 8272700: [macos] Build failure with Xcode 13.0 after JDK-8264848
f4be211: 8270041: Consolidate oopDesc::cas_forward_to() and oopDesc::forward_to_atomic()
b40e8f0: 8271951: Consolidate preserved marks overflow stack in SerialGC
7eccbd4: 8266519: Cleanup resolve() leftovers from BarrierSet et al
... and 75 more: https://git.openjdk.java.net/jdk/compare/cd1751c34e974683f3d2734c8ad5823a6ea27295...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-20T11:30:25Z

@walulyai Pushed as commit fb1dfc6.

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

8267185: Add string deduplication support to ParallelGC

b1fe77a

openjdk bot added the rfr Pull request is ready for review label Aug 11, 2021

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Aug 11, 2021

kimbarrett suggested changes Aug 16, 2021

View reviewed changes

Kim review

cd22f56

kimbarrett approved these changes Aug 16, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 16, 2021

fix indentation

ba73d85

albertnetymk reviewed Aug 19, 2021

View reviewed changes

albertnetymk review

90dec08

albertnetymk approved these changes Aug 19, 2021

View reviewed changes

kimbarrett suggested changes Aug 20, 2021

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Aug 20, 2021

kimbarrett suggested changes Aug 20, 2021

View reviewed changes

walulyai added 2 commits August 20, 2021 08:18

kim requested cleanup

bc1cca9

more clean up

2f2a2cd

albertnetymk approved these changes Aug 20, 2021

View reviewed changes

kimbarrett approved these changes Aug 20, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 20, 2021

openjdk bot closed this Aug 20, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8267185: Add string deduplication support to ParallelGC #5085

8267185: Add string deduplication support to ParallelGC #5085

walulyai commented Aug 11, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 11, 2021

openjdk bot commented Aug 11, 2021

mlbridge bot commented Aug 11, 2021 •

edited

Loading

kimbarrett Aug 11, 2021

kimbarrett Aug 16, 2021

kimbarrett Aug 16, 2021

kimbarrett Aug 16, 2021

kimbarrett Aug 16, 2021

kimbarrett Aug 16, 2021

kimbarrett Aug 16, 2021

kimbarrett left a comment

kimbarrett Aug 16, 2021

walulyai Aug 16, 2021

openjdk bot commented Aug 16, 2021 •

edited

Loading

albertnetymk left a comment

albertnetymk Aug 19, 2021

kimbarrett Aug 20, 2021

kimbarrett Aug 20, 2021

walulyai Aug 20, 2021

kimbarrett left a comment

walulyai commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

8267185: Add string deduplication support to ParallelGC #5085

8267185: Add string deduplication support to ParallelGC #5085

Conversation

walulyai commented Aug 11, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 11, 2021

openjdk bot commented Aug 11, 2021

mlbridge bot commented Aug 11, 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

kimbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Aug 16, 2021 • edited Loading

albertnetymk left a 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

kimbarrett left a comment

Choose a reason for hiding this comment

walulyai commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

walulyai commented Aug 11, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 11, 2021 •

edited

Loading

openjdk bot commented Aug 16, 2021 •

edited

Loading