RFR (S): 8026043: add regression test for JDK-8000831

Erik Helin erik.helin at oracle.com
Mon Nov 25 14:00:55 UTC 2013


Filipp,

On 2013-11-06 17:11, Filipp Zhinkin wrote:
> Hi,
>
> may I have a review on it?

Overall, the test looks fine. Only three small comments:
- Could you @run main/samevm? I think that you can actually remove @run
   tag, since AFAIK the default action for jtreg is:
        @run main/samevm NameOfTest
- The method ProcessTools.createVerboseJavaProcessBuilder is not
   available yet (it is in one of your other changes), can you use the
   existing ProcessTools.createJavaProcessBuilder for now?
- Could you make the garbage variable public?

Thanks for writing new tests!
Erik

> Thanks,
> Filipp.
>
> On 10/22/2013 12:40 PM, Filipp Zhinkin wrote:
>> Hi,
>>
>> I've done following changes:
>> - @build tag removed;
>> - 'garbage' array is now static field;
>> - added heap-sizing flags to avoid failures with options passed via
>> 'test.java.opts'.
>>
>> Updated webrev: http://cr.openjdk.java.net/~kshefov/8026043/webrev.02/
>>
>> Thanks,
>> Filipp.
>>
>> On 10/18/2013 07:33 PM, Filipp Zhinkin wrote:
>>> Hi Erik,
>>>
>>> thank you for watching at it!
>>>
>>> On 10/18/2013 06:59 PM, Erik Helin wrote:
>>>> Hi Filipp,
>>>>
>>>> thanks for adding new tests, really appreciate it!
>>>>
>>>> I have a couple of comments/questions about this test:
>>>> - Do you really need a @build tag?
>>> No, I don't.
>>>> - Could you please provide a comment above each regex with an example
>>>>    that shows what the regex matches?
>>> Yes, sure.
>>>> - This test will only run with the default GC (since no GC is
>>>>    specified). What do you think about additional run targets testing
>>>>    various GC combinations?
>>> The idea was to pass all VM options including different GC's via
>>> test.java.opts property.
>>> It makes test more flexible and allow us to test not only some fixed
>>> subset of GC combinations.
>>>> - Have you tried running the test with -Xcomp? The reason I'm asking is
>>>>    because I'm afraid that the compiler might be able to realize
>>>> that the
>>>>    GarbageProducer main method isn't doing anything.
>>> Yes, I've tried and that object allocation was not optimized out,
>>> but I agree with you, "garbage" should be static.
>>>>
>>>>    One thing that usually solves this is having garbage as a public
>>>>    static variable in the class.
>>>> - Are you using GarbageProducer since you want to trigger young GCs? If
>>>>    so, why only tests young GCs? Would the test works just as well
>>>> using
>>>>    System.gc() to tests full GCs?
>>> In fact, test running with MaxHeapSize=5m, so both minor and full GC
>>> happens.
>>> But there is another issue then  - I forgot to set up other heap
>>> sizing options,
>>> so test will fail when someone start if with, for example, -Xms10m.
>>>
>>> I'll fix that.
>>>
>>> Thanks,
>>> Filipp.
>>>> Thanks,
>>>> Erik
>>>>
>>>> On 2013-10-14, Filipp Zhinkin wrote:
>>>>> Hi,
>>>>>
>>>>> I've refactored options parsing, here is updated webrev:
>>>>> http://cr.openjdk.java.net/~kshefov/8026043/webrev.01/
>>>>>
>>>>> Note that I'm using ProcessTools.createVerboseJavaProcessBuilder
>>>>> from 8026047
>>>>> to make sure that VM output will not be suppressed or redirected to
>>>>> file.
>>>>>
>>>>> Thanks,
>>>>> Filipp.
>>>>>
>>>>> On 10/11/2013 08:22 PM, Tao Mao wrote:
>>>>>> Ok, thank you for heads-up. Then, I'll await your webrev update.
>>>>>>
>>>>>> Thanks.
>>>>>> Tao
>>>>>>
>>>>>> On 10/11/13 9:11 AM, Filipp Zhinkin wrote:
>>>>>>> Hi Tao,
>>>>>>>
>>>>>>> Sorry for typo. You're right, the correct one is
>>>>>>> http://cr.openjdk.java.net/~kshefov/8026043/webrev.00/.
>>>>>>>
>>>>>>> And I'll refactor it in the same way as fix for 8026047.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Filipp.
>>>>>>>
>>>>>>> On 10/11/2013 08:11 PM, Tao Mao wrote:
>>>>>>>> Hi Filipp,
>>>>>>>>
>>>>>>>> FYI, it has a wrong webrev. I guess it should be
>>>>>>>> http://cr.openjdk.java.net/~kshefov/8026043/webrev.00/
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Tao
>>>>>>>>
>>>>>>>> On 10/8/13 8:34 AM, Filipp Zhinkin wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I would like to get couple review on test, that cover issue
>>>>>>>>> fixed by 8000831 - Heap verification output
>>>>>>>>> incorrect/incomplete.
>>>>>>>>>
>>>>>>>>> Test verifies that options VerifyBeforeGC and
>>>>>>>>> VerifyAfterGCemit appropriate output if they are turned on
>>>>>>>>> and never
>>>>>>>>> emit corrupted output as it was before 8000831 fixed.
>>>>>>>>>
>>>>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8026043
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~kshefov/8026047/webrev.00/
>>>>>>>>> Testing: manual on local host, automated on various platforms.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Filipp.
>>>
>>
>



More information about the hotspot-gc-dev mailing list