RFR(S): 8159818: Convert IHOP_test to GTest

Dmitry Fazunenko dmitry.fazunenko at oracle.com
Tue Sep 27 11:55:59 UTC 2016


Hi Kirill,

The fix looks good to me. A couple of nits

1) Copyright year of test_g1IHOPControl.cpp could be 2015, 2016. Because 
the code existed before.
2) It would be awesome if you provide a brief summary (1 sentence) of 
what the tests are checking.
   I understand, this is out of the conversion scope, so I don't insist.

No need a separate webrev.

Thanks,
Dima

On 26.09.2016 17:55, Kirill Zhaldybin wrote:
> Thomas,
>
> Thank you for reviewing the fix!
>
> Here are a new WebRev: 
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8159818/webrev.01/
>
> Could you please let me know your opinion?
>
> Changes:
> 1. g1IHOPControl.hpp copyright updated
> 2. static void test_update parameters list reformatted.
> 3. Comment "Test could be only run with G1" changed to "Test requires G1"
> 4. Some reviewers are pretty strict about 80 symbols. Could you please 
> let me know if I need to change limit for the line lengths?
>
> Thank you.
>
> Regards, Kirill
>
> On 26.09.2016 11:33, Thomas Schatzl wrote:
>> Hi Kirill,
>>
>> On Thu, 2016-09-22 at 22:38 +0300, Kirill Zhaldybin wrote:
>>> Dear all,
>>>
>>> Could you please review this fix for 8159818?
>>>
>>> Two tests were converted to GTest.
>>> Since these tests require G1 GC, checks were added to prevent them to
>>> be
>>> run with other GCs.
>>> Comments "// @requires UseG1GC" were added to facilitate finding such
>>> tests.
>>>
>>> WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8159818/webr
>>> ev.00/
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8159818
>>    - g1IHOPControl.hpp needs copyright update
>>
>> test_g1IHOPControl.cpp:
>>
>> -   30 static void test_update(G1IHOPControl* ctrl, double alloc_time,
>>      31         size_t alloc_amount, size_t young_size, double
>> mark_time) {
>>
>> please follow usual hotspot style guidelines for formatting parameter
>> lists.
>>
>> -   40   // Test could be only run with G1
>>      75   // Test could be only run with G1
>>
>> -> "Test requires G1."
>>
>> -   95   const size_t settled_ihop1 = target_size
>>    96           - (young_size + alloc_amount1 / alloc_time1 *
>> marking_time1);
>>
>> when breaking lines (the whole file seems to have a strict ~80 chars
>> line limit, which we do not require), I prefer to have the next line
>> not begin with an operator (others may disagree, so ignore this if
>> wanted).
>> Anyway I think there is no need to limit the line lengths to 80 chars.
>>
>> Looks good otherwise.
>>
>> Thanks
>>    Thomas
>>
>



More information about the hotspot-gc-dev mailing list