RFR: 8254598: StringDedupTable should use OopStorage [v4]

[Zhengyu Gu]

On Fri, 14 May 2021 04:31:59 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change to the String Deduplication facility.  It is being
>> changed to use OopStorage to hold weak references to relevant objects,
>> rather than bespoke data structures requiring dedicated processing phases by
>> supporting GCs.
>> 
>> (The Shenandoah update was contributed by Zhengyu Gu.)
>> 
>> This change significantly simplifies the interface between a given GC and
>> the String Deduplication facility, which should make it much easier for
>> other GCs to opt in.  However, this change does not alter the set of GCs
>> providing support; currently only G1 and Shenandoah support string
>> deduplication.  Adding support by other GCs will be in followup RFEs.
>> 
>> Reviewing via the diffs might not be all that useful for some parts, as
>> several files have been essentially completely replaced, and a number of
>> files have been added or eliminated.  The full webrev might be a better
>> place to look.
>> 
>> The major changes are in gc/shared/stringdedup/* and in the supporting
>> collectors, but there are also some smaller changes in other places, most
>> notably classfile/{stringTable,javaClasses}.
>> 
>> This change is additionally labeled for review by core-libs, although there
>> are no source changes there.  This change injects a byte field of bits into
>> java.lang.String, using one of the previously unused padding bytes in that
>> class.  (There were two unused bytes, now only one.)
>> 
>> Testing:
>> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>> 
>> Locally (linux-x64) ran all of the existing tests that use string
>> deduplication with both G1 and Shenandoah.  Note that
>> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
>> currently fails, for reasons I haven't figured out but suspect are test
>> related.
>> 
>> - gc/stringdedup/   -- these used to be in gc/g1
>> - runtime/cds/SharedStringsDedup.java
>> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
>> - runtime/cds/appcds/sharedStrings/*
>> 
>> shenandoah-only:
>> - gc/shenandoah/jvmti/TestHeapDump.java
>> - gc/shenandoah/TestStringDedup.java
>> - gc/shenandoah/TestStringDedupStress.java
>> 
>> Performance tested baseline, baseline + stringdedup, new with stringdedup,
>> finding no significant differences.
>
> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into new_dedup2
>  - misc cleanups
>  - refactor Requests::add
>  - fix typo
>  - more comment improvements
>  - improve naming and comments around injected String flags
>  - fix some typos in comments
>  - New string deduplication

> The "merge from master" commit ([ccb9951](https://github.com/openjdk/jdk/commit/ccb99515d020785d7fe1cf9a1c247aeed775dc19)) doesn't build with Shenandoah. I've asked Zhengyu to take a look.

Just missing a parameter:

```diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp b/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
index ddaa66ccc14..93a067fa22d 100644
--- a/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
+++ b/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
@@ -57,7 +57,7 @@ ShenandoahInitMarkRootsClosure::ShenandoahInitMarkRootsClosure(ShenandoahObjToSc
 
 template <class T>
 void ShenandoahInitMarkRootsClosure::do_oop_work(T* p) {
-  ShenandoahMark::mark_through_ref<T, NO_DEDUP>(p, _queue, _mark_context, false);
+  ShenandoahMark::mark_through_ref<T, NO_DEDUP>(p, _queue, _mark_context, NULL, false);
 }```

src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:

> 312:   size_t _bucket_index;
> 313:   size_t _shrink_index;
> 314:   bool _grow_only;

Indentation

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

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