RFR (S): 8135200: Add White Box method that enumerates G1 old regions with less than specified liveness and collects statistics.

[Mikael Gerdin]

Hi Kirill,

On 2015-09-23 15:41, Kirill Zhaldybin wrote:
> Hi Thomas,
>
> Thank you for reviewing this fix.
>
> Please read comments inline.
>
> Regards, Kirill
>
> On 22.09.2015 16:17, Thomas Schatzl wrote:
>> "Hi Kirill,
>>
>> On Wed, 2015-09-09 at 21:10 +0300, Kirill Zhaldybin wrote:
>>> Hi!
>>>
>>> Could you please review a patch for JDK-8135200?
>>>
>>> I added White Box method that enumerates G1 old regions with less than
>>> specified liveness and collects statistics.
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8135200
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8135200/webrev.00/
>>> hotspot:
>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8135200/hotspot/webrev.00/
>>>
>>>
> New Webrev:
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8135200/webrev.01/
> hotspot:
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8135200/hotspot/webrev.01/

Please make the size_t members of the closure private, name them with a 
leading _ and snake_case and create getters for them to follow the usual 
style.

The jlong array handling:
  395   CHECK_JNI_EXCEPTION_(env, NULL);
  396   ThreadToNativeFromVM ttn(thread);
  397   result = env->NewLongArray(3);
  398   CHECK_JNI_EXCEPTION_(env, NULL);
  399   jlong count = (jlong)rli.totalCount;
  400   jlong totMemory = (jlong)rli.totalMemory;
  401   jlong memToFree = (jlong)rli.totalMemToFree;
  402   env->SetLongArrayRegion(result, 0, 1, &count);
  403   env->SetLongArrayRegion(result, 1, 1, &totMemory);
  404   env->SetLongArrayRegion(result, 2, 1, &memToFree);
  405   return result;

Could be rewritten to avoid the JNI API as follows (CHECK_NULL does an 
exception check and returns if the allocation throws an exception).

typeArrayOop result = oopFactory::new_longArray(3, CHECK_NULL);
result->long_at_put(0, rli.count());
result->long_at_put(1, rli.total_memory());
result->long_at_put(2, rli.memory_to_free());
return JNIHandles::make_local(env, result);


Thanks
/Mikael

>>
>>    some comments about the change:
>>
>> - there is some wrong assumption in the code:
>>
>> 370         if (size == reg_size) { // non-full regions are not included
>> to the mixed GC
>>   371           totalMemToFree += size - prev_live;
>>   372         }
>>
>> This is not true. G1 may try to evacuate non-full regions (regions
>> recently allocated into), but it is just unlikely.
> changed comments, since we don't need exact value, lowest estimation
> fits too.
>>
>> - lines 374-376 look like debug code. Are they required for some reason?
> fixed, output deleted.
>>
>> As for the follow-up JDK-8129579 New tests for G1 Mixed GC are required"
>> I am not sure if it is actually useful to try to test them. I mean,
>> these are artifacts of the current implementation, and as long as mixed
>> gc goals are met imo mixed gc may do anything to achieve its goal.
>>
>> Thanks,
>>    Thomas
>>
>>
>