RFR(L): 8029075 - String deduplication in G1

[Per Liden]

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/

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

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

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

/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
>