RFR: 8267185: Add string deduplication support to ParallelGC

[Kim Barrett]

On Wed, 11 Aug 2021 13:56:09 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi all,
> 
> Please review this change to add string deduplication support to ParallelGC.
> 
> Testing: All string deduplication tests, and Tier 1-5.

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp line 143:

> 141: 
> 142:   void safepoint_synchronize_begin();
> 143:   void safepoint_synchronize_end();

I see no callers of these new functions.

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp line 289:

> 287:       push_contents(new_obj);
> 288: 
> 289:       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?

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp line 290:

> 288: 
> 289:       if (psStringDedup::is_candidate_from_evacuation(o->klass(), new_obj->age(), new_obj_is_tenured)) {
> 290:         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.

src/hotspot/share/gc/parallel/psStringDedup.hpp line 41:

> 39:                                            uint age,
> 40:                                            bool obj_is_tenured) {
> 41:     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.

src/hotspot/share/gc/shared/stringdedup/stringDedupConfig.cpp line 119:

> 117:   if (!UseStringDeduplication) {
> 118:     return true;
> 119:   } 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.

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

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