RFR(L): 8029075 - String deduplication in G1

[Bengt Rutisson]

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