RFR (L): 8024319: Add regression tests on documented GC-related -XX:+Print* options
denis kononenko
denis.kononenko at oracle.com
Fri Jan 23 12:23:01 UTC 2015
Hi Bengt,
Thank you for sharing your opinion.
On 23.01.2015 12:02, Bengt Rutisson wrote:
>
> 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.
As I understood from your and Dima's discussion you disagreed with two
points:
1) introducing AbstractPrintGCTest;
2) spawning another process with built-in jtreg's functionality;
1) When I finished my first implementation I found that I had to
introduce AbstractPrintGCTest class anyway. Just because of the the
single responsibility principle and to avoid code duplication in the
tests. The main responsibility of this class is to provide the log
output to the tests, that is it. The only difference between the
implementations is that the old one had the code to spawn a process
while the new simply passes this responsibility to jtreg. Look at this
class like at 'setup' method in JUnit test, we're just testing the
output and we're not interested in how we get this output. So, my
opinion is that this implementation concern shall be separated from the
actual testing.
2) I see two major benefits from using ProcessTools. The first one is
that I could capture the output without writing it to the logfile, that
is, the test is going to be more simple and stable. The second one -- I
could run the test without having jtreg. The disadvantage is that
ProcessTools still require the native library, it slightly complicates
manual testing, I cannot simply pick up a test and run/debug it on
another platform just to see how it works.
Ideally I'd prefer to have more finer control over GC logger's settings,
in particular, it would be great if I could setup the output stream for
the logger. In such case the tests would be simplified dramatically, no
log files, no other processes.
Thank you,
Denis.
>
> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list