RFR(L): 8029075 - String deduplication in G1

[Bengt Rutisson]

Hi Per,

On 2014-03-07 15:13, Per Liden wrote:
> Hi,
>
> Here's an updated webrev based on the feedback from Bengt and Thomas.
>
> http://cr.openjdk.java.net/~pliden/8029075/webrev.1/
>
> Diffs against previous webrev available here:
> http://cr.openjdk.java.net/~pliden/8029075/webrev.1-bengt_review_fixes/
> http://cr.openjdk.java.net/~pliden/8029075/webrev.1-thomas_review_fixes/

Thanks for providing these diffs. Made it easy to review.

Looks good to me.

>
> And as suggested, two changes in the initial webrev were broken out 
> into separate CRs, available here:

Great! Thanks for splitting these out.

>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8036672
> Webrev: http://cr.openjdk.java.net/~pliden/8036672/webrev.0/

Looks good.

>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8036673
> Webrev: http://cr.openjdk.java.net/~pliden/8036673/webrev.0/

Looks good.

Bengt


>
> /Per
>
> On 03/05/2014 11:00 AM, Bengt Rutisson wrote:
>>
>> Hi Per and everyone else,
>>
>> Overall this looks good to me. Me and Per went through this patch
>> together and here are the notes from this code walk through.
>>
>> There are two small bug fixes included in the change. It would be good
>> to separate these out to separate changes. We can review them together
>> with the main change, but having them as separate changesets will make
>> it easier to backport them if necessary.
>>
>>
>> stringDedup.hpp
>> The comments are not that helpful to me. I would suggest to either
>> remove them or make more descriptive.
>>
>> UseG1GC && UseStringDeduplication
>> Only allow UseStringDeduplication to be true if UseG1GC is true instead
>> of checking both?
>>
>> G1CollectedHeap::tear_down_region_sets() and
>> G1CollectedHeap::rebuild_region_sets()
>> Please add an RFE to clean these up as the naming and implementations
>> don't match anymore.
>>
>> StringDedupTable::trim()
>> rename to trim_cache?
>>
>> stringDedup.cpp
>> Two occurrences of:
>>    assert(UseG1GC, "String deduplication not available with G1");
>> The "not" should be a "only".
>>
>> Dummy statistics from interned stings is ignored. Please add an RFE to
>> try to include this statistics and possibly reduce the "known" count.
>>
>>
>> Add comments explaining why resize and rehash is not allowed for these
>> code paths:
>>
>> void StringDedup::unlink(BoolObjectClosure* is_alive) {
>>    unlink_or_oops_do(is_alive, NULL, false /* allow_resize_and_rehash 
>> */);
>> }
>>
>> void StringDedup::oops_do(OopClosure* keep_alive) {
>>    unlink_or_oops_do(NULL, keep_alive, false /* allow_resize_and_rehash
>> */);
>> }
>>
>> TestStringDeduplicationTools.java
>> verifyStrings  put the sleep at the end to possibly speed up the test a
>> little.
>>
>> InternedTest
>> Comments to explain what it does.
>>
>> testYoungGC - check that no full GCs happen
>> testFullGC - check that no young GCs happen
>>
>> testMemoryUsage - add "memoryUsageWithDedup >
>> memoryUsageWithDedupExpected" to the exception message
>>
>>
>> Thanks,
>> Bengt
>>
>>
>> On 3/3/14 2:33 PM, Per Liden wrote:
>>> Hi,
>>>
>>> Could I please have this patch reviewed.
>>>
>>>
>>> Summary
>>> -------
>>> This patch implements JEP 192 - String deduplication in G1. The goal
>>> of string deduplication is to reduce the Java heap live-data set by
>>> enhancing the G1 garbage collector so that duplicate instances of
>>> String are automatically and continuously deduplicated.
>>>
>>> I'd like to refer to the JEP for a more detailed description of this
>>> feature, the motivation for it, the expected benefit, how it's
>>> implemented, etc.
>>>
>>> This patch is mainly G1-related, but also touches a few runtime files.
>>>
>>> Webrev: http://cr.openjdk.java.net/~pliden/8029075/webrev.0/
>>>
>>> JEP: http://openjdk.java.net/jeps/192
>>>
>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8029075
>>>
>>>
>>> Testing
>>> -------
>>> * JTreg - The patch includes the following 8 new tests:
>>>   - TestStringDeduplicationYoungGC.java: Tests deduplication during
>>> Young GC.
>>>   - TestStringDeduplicationFullGC.java: Tests deduplication during
>>> Full GC.
>>>   - TestStringDeduplicationAgeThreshold.java: Tests both valid and
>>> invalid age threshold settings.
>>>   - TestStringDeduplicationInterned.java: Tests that interned strings
>>> are deduplication before being interned.
>>>   - TestStringDeduplicationTableRehash.java: Stresses the hashtables
>>> ability to rehash all entries.
>>>   - TestStringDeduplicationTableResize.java: Stresses the hashtables
>>> ability to resize itself.
>>>   - TestStringDeduplicationMemoryUsage.java: Tests heap reduction when
>>> string deduplication is enabled.
>>>   - TestStringDeduplicationPrintOptions.java: Tests command line 
>>> options.
>>>
>>> * Stress testing:
>>>   - Kitchensink
>>>   - GCBasher
>>>
>>> * Regression testing:
>>>   - JCK
>>>   - vmTestbase
>>>   - Bigapps
>>>
>>> * Large scale benchmarks to test heap reduction and performance impact:
>>>   - FA CRM Sales Op. Flow
>>>   - FA DSS
>>>
>>> * The following benchmarks have been executed to verify that this
>>> feature doesn't impact performance when disabled (even when disabled
>>> there are still some "if (UseStringDeduplication)" executed in some
>>> hot paths).
>>>   - SPECjbb2005
>>>   - SPECjbb2013
>>>   - SPECjvm2008-XML
>>>
>>> * Various ad-hoc tests and microbenchmarks were also written and
>>> executed during the course of the development.
>>>
>>>
>>> Thanks!
>>> /Per
>>
>