On Fri, 7 May 2021 08:28:43 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 incrementally with three additional
> commits since the last revision:
>
> - more comment improvements
> - improve naming and comments around injected String flags
> - fix some typos in comments
Just some minor comments.
src/hotspot/share/gc/shared/stringdedup/stringDedup.cpp line 171:
> 169: // Store the string in the next pre-allocated storage entry.
> 170: oop* ref = _buffer[--_index];
> 171: _buffer[_index] = nullptr;
It's not really needed to clear the slot with null, right?
src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 51:
> 49: }
> 50:
> 51: StringDedup::Processor::~Processor() {}
Why the empty destructor? Could we just not have it?
src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 92:
> 90: GrowableArrayCHeap<TableValue, mtStringDedup> _values;
> 91:
> 92: void adjust_capacity(int new_length);
Nit: diff arg name in declaration and implementation, `new_length` vs
`new_capacity`.
src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 295:
> 293:
> 294: protected:
> 295: CleanupState() {}
Is it possible to use `= default` here? Just like the destructor.
src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:
> 312: size_t _bucket_index;
> 313: size_t _shrink_index;
> 314: bool _grow_only;
Seems unused.
src/hotspot/share/memory/universe.cpp line 1153:
> 1151: ResolvedMethodTable::verify();
> 1152: }
> 1153: if (should_verify_subset(Verify_StringDedup) &&
> StringDedup::is_enabled()) {
Maybe drop the `is_enabled()` check in the `if` to keep the consistency with
other `if`s?
-------------
Marked as reviewed by ayang (Committer).
PR: https://git.openjdk.java.net/jdk/pull/3662
