RFR: 8254598: StringDedupTable should use OopStorage

[Kim Barrett]

On Tue, 27 Apr 2021 22:30:04 GMT, Coleen Phillimore <coleenp 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.
>
> src/hotspot/share/classfile/javaClasses.inline.hpp line 77:
> 
>> 75: 
>> 76: uint8_t* java_lang_String::flags_addr(oop java_string) {
>> 77:   assert(_initialized, "Mut be initialized");
> 
> typo: must

Fixed locally.

> src/hotspot/share/runtime/globals.hpp line 1994:
> 
>> 1992:                                                                             \
>> 1993:   product(uint64_t, StringDeduplicationHashSeed, 0, DIAGNOSTIC,             \
>> 1994:           "Seed for the table hashing function; 0 requests computed seed")  \
> 
> Should these two really be develop() options?

StringDeduplicationResizeALot is used by a test that we want to run against release builds too.  If StringDeduplicationHashSeed isn't a diagnostic option (so available in release builds), I'd be more inclined to just remove it than to make it a develop option.

> src/hotspot/share/runtime/mutexLocker.cpp line 239:
> 
>> 237:     def(StringDedupQueue_lock      , PaddedMonitor, leaf,        true,  _safepoint_check_never);
>> 238:     def(StringDedupTable_lock      , PaddedMutex  , leaf + 1,    true,  _safepoint_check_never);
>> 239:   }
> 
> Why weren't these duplicate definitions?  This is disturbing.

These are assignments, not definitions.  Only one of the assignments was being performed because only one of G1 or Shenandoah will be selected.  (Not that it matters anymore after this change.)

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

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