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

Filipp Zhinkin filipp.zhinkin at oracle.com
Tue Nov 26 09:48:18 UTC 2013


Eric,

thanks for looking at it!

I've fixed all issues that you've listed below.
Here is updated webrev:
http://cr.openjdk.java.net/~kshefov/8026043/webrev.03/

Thanks,
Filipp.

On 11/25/2013 06:00 PM, Erik Helin wrote:
> 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