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

[Bengt Rutisson]

Hi Dima,

Sorry top posting, but the email thread is getting pretty long. :)

You wrote:

"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 
"

I already suggested to use this approach in an earlier email. This is 
much more readable and less error prone than the new framework that you 
proposed. So, I much prefer the tests to be implemented this way.

Bengt


On 2015-01-22 14:57, Dmitry Fazunenko wrote:
>
> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>