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

[Kirill Zhaldybin]

Mikael, Thomas,

Thank you for your comments.

Here are new webrevs:
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8135200/webrev.02/
hotspot:
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8135200/hotspot/webrev.02/

Regards, Kirill

On 24.09.2015 12:00, Mikael Gerdin wrote:
> 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
>>>
>>>
>>
>