Request for sponsor - JDK-8031538

[Thomas Schatzl]

Hi,

On Wed, 2014-01-22 at 16:56 -0800, Staffan Friberg wrote:
> Hi,
> 
> I have created the a patch for JDK-8031538 - G1 eden usage is sometimes 
> higher than target eden (printed Eden size) [1].
> 
> Since I'm not author I can't upload public WebRevs or request reviews, 
> so I would like to ask if someone could sponsor this change and push it 
> through the process.

I can sponsor the patch and uploaded a webrev at
http://cr.openjdk.java.net/~tschatzl/8031538/webrev/

This will be your second contribution then, allowing you to become
author :)

> I have attached the small patch to this email. The patch make sure that 
> the nursery target size calculation takes into account any regions that 
> might currently be used for eden and not only the size of the survivor 
> space.
> 
> [1] - https://bugs.openjdk.java.net/browse/JDK-8031538

I think there is a small problem after the change: when revising the
young target list length concurrently, the code in lines 550/551 more or
less make sure that the young gen will only ever increase because
absolute_min_length is always young_list_length() + 1.

So this could mean that if this length is updated really frequently, we
may exhaust the entire heap. E.g. recalculation of this length increases
the amount of eden -> allocation can continue -> before OOM there is
another recalculation -> goto start.

 542   // This is how many young regions we already have (survivors +
       // eventual eden)
 543   // The young list contain all survivors regions and any eden regions

Maybe add an explanation that during a GC pause the young list contains
only survivor regions at this point.

 544   uint base_min_length = _g1->young_list()->length();
 545   // This is the absolute minimum young length, which ensures that we
 546   // can allocate one eden region in the worst-case.
 547   uint absolute_min_length = base_min_length + 1;

I think the unconditional +1 is wrong. It should probably only add this single region if there are not yet eden regions added. I think you could get this information (whether the young list already contains an eden region) from YoungList, by subtracting length() with survivor_length(). If the result is > 0, there are eden regions in the list already. It's possibly best to add a method that returns that boolean value.

 548   uint desired_min_length =
 549                      calculate_young_list_desired_min_length(base_min_length);
 550   if (desired_min_length < absolute_min_length) {  // <---- !!
 551     desired_min_length = absolute_min_length;
 552   }

^^^ ---- maybe above can also be replaced by a MAX2(desired_min_length, absolute_min_length) statement. It's shorter and more readably imo.

Given that I think the "problem" was really just a reporting problem.

Thanks a lot for digging into this,
  Thomas