On Fri, 14 May 2021 04:31:59 GMT, Kim Barrett <kbarr...@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