RFR(S): 8150865: SQE test: GC unified logging: check that dynamic log level doesn't break anything

[Dmitry Fazunenko]

Jesper,

thanks for answer.

Kirill,

would you move the test back and exclude it from the hotspot_fast_gc_2  
group
(hotspot/test/TEST.groups)

Thanks,
Dima

On 10.05.2016 20:42, Jesper Wilhelmsson wrote:
> Hi Dima,
>
> I would prefer to have the test in gc/logging as long as it doesn't 
> interfere with test selection in nightlies etc.
>
> It has Stress in its name so that should be enough to indicate what 
> kind of test it is. If there is a lot of randomness in a test a 
> comment in the test would help a lot when investigating a potential 
> future failure in the test.
>
> Thanks,
> /Jesper
>
> Den 10/5/16 kl. 18:32, skrev Dmitry Fazunenko:
>> Hi Jesper,
>>
>>
>> On 10.05.2016 18:51, Jesper Wilhelmsson wrote:
>>>
>>>
>>> Den 10/5/16 kl. 08:12, skrev Dmitry Fazunenko:
>>>> Hi Kirill,
>>>>
>>>> The test looks very good!
>>>> Some comments which will make it look even better :)
>>>>
>>>> 1) Please remove dependency on G1. Unified logging is a common
>>>> feature and should be tested in all collectors.
>>>> You can keep two kinds of objects: small and large. To avoid 
>>>> confusion,
>>>> "large" or "huge" term could be used instead of "humongous".
>>>>
>>>> 2) Because this is a stress test, please move it: gc/logging --> 
>>>> gc/stress
>>>
>>> I assume this is done to make it easier to select tests in different 
>>> test lists?
>>>
>>> In general I would prefer to sort the tests based on what they test, 
>>> not what
>>> kind of test it is. It's easier to find the right test when one has 
>>> made a
>>> change in some part of the code if all tests related to that code is 
>>> in the
>>> same place.
>>>
>>> Updating the test lists to manually include/exclude the stress tests 
>>> is surely
>>> a tedious job, but it is something that would be done once for every 
>>> new test.
>>> Trying to find the right set of tests to run on a change is a 
>>> problem that
>>> every developer is facing more or less on a daily basis. I think it 
>>> is more
>>> important to make this the easy task.
>>
>> You rose a very good question: where to store stress tests.  And I 
>> agree with
>> you, it's better to store logging tests in the logging directory.
>> And use gc/stress for storing general purpose tests like GCOld.
>>
>> But this particular test (by Kirill) doesn't check any specific logging
>> assertion, it just checks that VM doesn't crash trying to log something.
>> Because it's based on random, passing of the test doesn't guarantee 
>> "no bugs".
>> I think it's better to locate it in gc/stress/
>> to emphasize that a failure is not necessary caused by the resent 
>> changes, but
>> may exist before.
>>
>> If you still believe, the best test for that test is gc/logging, I 
>> will not
>> object :)
>>
>>
>> Kirill,
>>
>> New version looks good to me, thanks for the prompt addressing my 
>> comments.
>> I found one more minor change which will make the test stronger:
>>
>>    String logLevelCommand = "what='gc=" + LOG_LEVELS[logLevel] + "'";
>> -->
>>    String logLevelCommand = "what='gc*=" + LOG_LEVELS[logLevel] + "'";
>>
>>
>> I don't need a separate review.
>>
>> Thanks,
>> Dima
>>
>>
>>>
>>> /Jesper
>>>
>>>
>>>>
>>>>
>>>> 3)
>>>>
>>>> please add:
>>>>  * @key gc stress
>>>>
>>>> you don't need:
>>>>  * @build gc.logging.TestUnifiedLoggingSwitchStress
>>>>
>>>>
>>>> 4) Other comments are rather minor recommendations:
>>>>
>>>>     private final int filesCount;
>>>>     private final String fileNamePrefix;
>>>> -->
>>>>     private final int logCount;  // how many various log files will 
>>>> be used
>>>>     private final String logFilePrefix;  // name of log file will be
>>>> logFilePrefix + index
>>>>
>>>>
>>>>
>>>>     System.out.format("Diagnostic command vmLog with arguments 
>>>> %s,%s returned
>>>> not empty output %s\n",
>>>> -->
>>>>     System.out.format("WARNING: Diagnostic command vmLog with 
>>>> arguments %s,%s
>>>> returned not empty output %s\n",
>>>>
>>>>     LinkedList<Thread> threads = new LinkedList<>();
>>>> -->
>>>>     List<Thread> threads = new LinkedList<>();
>>>>
>>>>     Calendar calendar = Calendar.getInstance();
>>>>     long startTime = calendar.getTimeInMillis();
>>>> -->
>>>>     long startTime = System.currentTimeMillis()
>>>>
>>>>
>>>>     while (Calendar.getInstance().getTimeInMillis() - startTime < 
>>>> duration) {
>>>>         Thread.yield();
>>>>     }
>>>> -->
>>>>     while (System.currentTimeMillis() - startTime < duration) {
>>>>         Thread.sleep(1000);
>>>>     }
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Dima
>>>>
>>>> On 06.05.2016 14:10, Kirill Zhaldybin wrote:
>>>>> Dear all,
>>>>>
>>>>> Could you please review this fix for 8150865?
>>>>>
>>>>> The test creates two types of threads: the first type stresses gc 
>>>>> and the
>>>>> second type switches gc unified loggingl level/file output.
>>>>> The test is a stress test and it is expected that it finishes 
>>>>> normally (no oom
>>>>> or crash).
>>>>>
>>>>> WebRev: 
>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8150865/webrev.00/
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8150865
>>>>>
>>>>> Thank you.
>>>>>
>>>>> Regards, Kirill
>>>>
>>