RFR (L): 8024319: Add regression tests on documented GC-related -XX:+Print* options

[Dmitry Fazunenko]

On 22.01.2015 15:51, Bengt Rutisson wrote:
>
> Hi Dima,
>
> On 2015-01-22 12:42, Dmitry Fazunenko wrote:
>> Hi Bengt!
>>
>> Thanks for looking at the tests and sharing your thoughts!
>>
>> Yes, it was considered to use ProcessTool but we decided to introduce 
>> AbstractPringGCTest class.
>> Our thoughts:
>> - log generation and log analysis are two different tasks, and they 
>> should logically separated:
>
> Ok, but that's how it is with ProcessTool as well. Only it is much 
> clearer how the output you analyze is connected to the process you ran 
> if you use ProcessTool. I don't like how using AbstractPrintGCTest 
> requires that you name the -Xloggc file correct.

It's a very little concern. When you develop code you need to use 
variables declared previously. If you make a mistake the compiler will 
tell you.
The same is here, if you make the test will fail when you try to run it.

>
>>     * starting GCTask with various options  (one line @run 
>> main/othervm ... GCTask)
>
> GCTask is fine. The name is a bit odd, but I understand the need for a 
> common class to do allocations to trigger GC.

It's ok to rename.

>
>>     * log analysis (the java code within sources)
>
> Right. And you use the OutputAnalyzer just like you would if you were 
> using ProcessTool.
>
>> this increases readability
>
> I disagree that adding a new abstract class increases readability.

In this case the abstract class hides the code which not important for 
the test logic.
When you look at the test source you see only what is related to that 
particular test:

http://cr.openjdk.java.net/~eistepan/dkononen/8024319/webrev.00/raw_files/new/test/gc/logging/TestPrintAdaptiveSizePolicyParallelGCEnabled.java



>
>> - it's possible to provide several checkers for the same log
>>     @run main/othervm ... GCTask
>>     @run main TestCheck1
>>     @run main TestCheck2
>
> You can grab the output from the ProcessTool and pass it to several 
> checkers too. I don't see the differences.
>
>> ...
>>     @run main TestCheck3
>>   or provide the same check for several logs (not currently implemented)
>>     @run main/othervm ... -loggc:log1 GCTask
>>     @run main/othervm ... -loggc:log2 GCTask
>>     @run main/othervm ... -loggc:log3 GCTask
>>     ...
>>     @run main TestCheck log1 log2 log3
>
> Again, I don't see how AbstractPringGCTest is any different here 
> compared to just grabbing the output from the ProcessTool.
>
>>
>> - writing log to a dedicated file will guarantee, that program output 
>> and the GC log will not be mixed up.
>>   (not valid argument for that particular case, bun in general that's 
>> true)
>
> That's a valid point. But it shouldn't be much of a problem since the 
> tests are under our control. It is up to us what we log on System.out.
>
>>
>> - using @run main/othervm will allow to *not ignore* external options 
>> (jtreg -vmoptions:...), that makes such tests applicable for wider 
>> range of configurations and check, that for example -Xcomp doesn't 
>> turn off PrintGCDetails...
>
> You can pass the external options on to ProcessTool.

Passing external option in the each test?.. It will duplicate jtreg 
functionality and bring extra lines.

>
>>
>>
>> Regarding AbstractPringGCTes: it doesn't duplicate any functionality, 
>> it just reads content of a text file.
>
> Yes, but getting the output from a process is what ProcessTool does. 
> So, it duplicates the functionality of getting the GC output.

Reading text file content takes one line of code. I think we can afford 
this duplication.

>
>>
>> Regarding ProcessTools. Yes, it's possible to develop tests with this 
>> library. This library itself is very good and powerful. But I 
>> personally would not recommend using it for test development because 
>> it duplicates harness functionality. Unfortunately, jtreg currently 
>> doesn't provide all functionality required for VM testing and we have 
>> to use ProcessTools as workaround.
>> And people already got used to ProcessTools and like this style. But 
>> in long term, there will be the same problem with support of such 
>> tests, as we experience now with tests written in shell. Indeed, 
>> using ProcessTools is like using java for shell scripting.
>
> The long term support will be made more complex if we introduce yet 
> another way of doing the same thing.

Absolutely! ProcessTools  is another way of starting new VM, "@run 
othervm"  already does it. What I suggest is to stop using alternative 
ways to start VM and rely on the harness.

BTW, I don't see anything new in the suggested approach.

>
>>
>> Returning to Denis' tests. They intentionally do not use 
>> ProcessTools. They could be used as example demonstrating an 
>> alternative approach.
>
> Yes, I think it would be interesting to see what the tests would look 
> like based on ProcessTool.

It will look like many others tests written with ProcessTools:
http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/file/e6a0cfbfdc9a/test/gc/g1/TestGCLogMessages.java

you already get used to this file and find it convenient. But it's 
really hard to understand the logic.

Thanks,
Dima


>
> Thanks,
> Bengt
>
>>
>> Thanks,
>> Dima
>>
>>
>>
>> On 22.01.2015 10:26, Bengt Rutisson wrote:
>>>
>>> On 2015-01-22 08:20, Bengt Rutisson wrote:
>>>>
>>>> Hi Denis,
>>>>
>>>> Not a full review, but I have a question.
>>>>
>>>> It seems like the AbstractPrintGCTest is kind of duplicating what 
>>>> ProcessTools already does. Have you considered using 
>>>> ProcessTools.createJavaProcessBuilder(..) instead of the @run 
>>>> commands to automatically get the process control and log support 
>>>> instead of introducing the AbstractPrintGCTest class?
>>>
>>> Here's an example of how I mean that you can use 
>>> ProcessTools.createJavaProcessBuilder() instead:
>>>
>>> http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/file/e6a0cfbfdc9a/test/gc/g1/TestGCLogMessages.java 
>>>
>>>
>>> Bengt
>>>
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>> On 2015-01-20 16:49, denis kononenko wrote:
>>>>> Hi All,
>>>>>
>>>>> Could you please review new tests on GC-related -XX:Print options.
>>>>>
>>>>> Webrev link: 
>>>>> http://cr.openjdk.java.net/~eistepan/dkononen/8024319/webrev.00/
>>>>> Bug id: https://bugs.openjdk.java.net/browse/JDK-8024319
>>>>> Testing: automated
>>>>> Description:
>>>>>
>>>>> There is a group of new tests to test the following GC options:
>>>>>
>>>>> -XX:+-PrintAdaptiveSizePolicy
>>>>> -XX:+-PrintGCApplicationConcurrentTime
>>>>> -XX:+-PrintGCApplicationStoppedTime
>>>>> -XX:+-PrintGCDateStamps
>>>>> -XX:+-PrintGCDetails
>>>>> -XX:+-PrintGC
>>>>> -XX:+-PrintGCTimeStamps
>>>>> -XX:+-PrintTenuringDistribution
>>>>>
>>>>> Each of the tested options has a pair of corresponding tests, one 
>>>>> is for testing an enabled option and another for disabled. The 
>>>>> tests are simple parsers of GC's logger output and looking for a 
>>>>> specific marker corresponding to the tested option. The output is 
>>>>> provided by another process which is implemented in GCTask.java. 
>>>>> It's necessary because we have to guarantee that GC's log has been 
>>>>> completely written and committed to the filesystem before we start 
>>>>> analyzing it. The most obvious solution is to politely finish the 
>>>>> writing process. Thus the every test spawns an auxiliary process 
>>>>> which produces GC's log file, waits for its finish, loads the 
>>>>> output and then performs actual testing. These steps are 
>>>>> implemented with jtreg's annotations and a helper class which can 
>>>>> be found AbstractPrintGCTest.java. This class encapsulates reading 
>>>>> GC's log output from the log file and provides that output to the 
>>>>> tests.
>>>>>
>>>>> To get GC's logger working GCTask forces the garbage collecting 
>>>>> process. It attempts to consume all memory available to the young 
>>>>> generation by creating a lot of unreferenced objects. Sooner or 
>>>>> later the garbage collector shall be invoked. In favor of 
>>>>> performance the task is implemented to be ran with a small memory 
>>>>> size less or equal to 128 megabytes. This is excplicitly specified 
>>>>> with -Xmx JVM's option in jtreg's annotations.
>>>>>
>>>>> Please note that some options work for specific GCs only. To 
>>>>> prevent them from being executed against wrong GC jtreg's 
>>>>> annotations and groups are used.
>>>>>
>>>>> Thank you,
>>>>> Denis.
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>