RFR (M): 8027959: Investigate early reclamation of large objects in G1

[Thomas Schatzl]

Hi Bengt,

  thanks for the review.

On Wed, 2014-07-16 at 14:35 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> Thank for fixing this! Overall it looks very good.
> 
> I had an offline conversation with Thomas about using more explicit 
> testing for humongous objects instead of relying on failed allocations 
> in PLABs. We need to evaluate that from a performance perspective.
> 
> Some other minor comments:
> 
> 
> g1_globals.hpp
> Indentation of the trailing \ looks wrong. At least in the webrev.

Fixed.

> 
> g1CollectedHeap.hpp
> 
>    void set_in_cset(uintptr_t index) { assert(get_by_index(index) != 
> IsHumongous, "Should not overwrite InCSetOrHumongous value"); 
> set_by_index(index, InCSet); }
> 
> I think the assert message should be: "Should not overwrite IsHumongous 
> value"

Fixed, thanks.


> g1CollectedHeap.cpp
> 
> Style comment (not a strong opinion here). I think I would prefer to 
> move the check for G1ReclaimDeadHumongousObjectsAtYoungGC into inside 
> clear_humongous_is_live_table() rather than having it in gc_prologue(). 

I think I fixed that.

> Also, can clear_humongous_is_live_table() skip the clearing if 
> _has_humongous_reclaim_candidates is false?

Fixed.

> 
> Similarly, I would prefer to move the 
> G1ReclaimDeadHumongousObjectsAtYoungGC tests from 
> do_collection_pause_at_safepoint() into 
> register_humongous_regions_with_in_cset_fast_test() and 
> eagerly_reclaim_humongous_regions().

Done.

> 
> 
> The test in TestGCLogMessages just tests the syntax of the new log 
> messages. Is it worth adding a test that actually verifies that 
> humongous objects are reclaimed?

I added a test based on a test case I used for developing this feature
(http://cr.openjdk.java.net/~tschatzl/8027959/webrev.0_to_1/test/gc/g1/TestEagerReclaimHumongousRegions.java.html) It may be somewhat unstable though because it relies on how GCs are triggered in the VM and some timing, but it seems to work.
Basically, with the feature enabled, it should not trigger any full gcs,
while it triggers many with it disabled. The test detects that, and
concludes based on that that the feature is working.

Apart from that, due to the high turnaround of humongous objects it is a
good test to detect other errors.

Btw, I fixed another issue: when the feature has been enabled but there
were no candidates, the given times contained initialization data, which
messed up output.

Diff webrev
http://cr.openjdk.java.net/~tschatzl/8027959/webrev.0_to_1
New webrev:
http://cr.openjdk.java.net/~tschatzl/8027959/webrev.1

Thanks,
  Thomas