RFR (L): 8024319: Add regression tests on documented GC-related -XX:+Print* options
denis kononenko
denis.kononenko at oracle.com
Thu May 21 11:50:42 UTC 2015
Hi Jesper,
The new version relies upon a new testlibrary functionality which has
not been approved yet. I have to fix a few review findings and it's
going to happen very soon.
Thank you,
Denis.
On 21.05.2015 12:01, Jesper Wilhelmsson wrote:
> Hi Denis,
>
> Resurrecting this thread since it has been out for review for almost
> two years and it would be nice to get rid of this issue.
>
> It seems the latest version published had a few issues. Is there a new
> version available for review?
>
> /Jesper
>
>
> Bengt Rutisson skrev den 26/1/15 10:34:
>>
>> Hi Denis,
>>
>> On 2015-01-23 13:23, denis kononenko wrote:
>>>
>>> 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;
>>
>> Yes, I don't like using inheritance to get functionality like that.
>> Normally you
>> want to use aggregation instead.
>>
>>> 2) spawning another process with built-in jtreg's functionality;
>>
>> I am not against using JTeg functionality. What I object to is that
>> you invent a
>> new way of managing processes when we already have one. For your test
>> you need
>> two processes. You choose to let JTreg spawn both processes and you
>> make them
>> communicate via a file name that the compiler does not know about and
>> can not
>> verify. If you are lucky you will detect a spelling mistake when you
>> run the test.
>>
>> All our other tests use JTreg @run for the verification process and use
>> ProcessTool for the spawning the log producer process. The
>> communication between
>> the processes is handled by ProcessTool which basically removes the
>> spelling
>> mistake issue or at least catch it at compile time instead of having
>> to wait
>> until you run the test.
>>
>>>
>>> 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.
>>
>> First, you don't have to use inheritance to get rid of code
>> duplication. There
>> are other options. In this case you could use aggregation instead. Turn
>> AbstractPrintGCTest in to a utility class and have the test create an
>> instance
>> of it instead of inheriting from it. If you do that you can do other
>> simplification to make the test more stable. Such as turning the the
>> file name
>> into a parameter that you pass to the utility class to eliminate the
>> spelling
>> mistake issue that I mentioned. But if you do that I think you will
>> find that
>> you have pretty much re-invented ProcessTool again.
>>
>> Also, avoiding code duplication is a good thing but for tests
>> readability is
>> even more important. Having a few extra lines in the test can save a
>> lot of time
>> if you don't have to jump to a parent class to understand what is
>> going on.
>> There is a balance there of course, but personally I prefer a bit of
>> code
>> duplication in the tests than having to jump between a lot of classes
>> to figure
>> out what is going on.
>>
>>>
>>> 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.
>>
>> Another big advantage is that all our other log parsing tests use
>> ProcessTool so
>> it will make things consistent and faster to read if it is being used.
>>
>>> 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.
>>
>> Agreed, but it is not much easier to run a test stand alone if it
>> depends on a
>> lot of parent classes.
>>
>>>
>>> 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.
>>
>> This sounds like good input to the unified logging project
>> (http://openjdk.java.net/jeps/158). Maybe you should discuss with
>> them about
>> your needs?
>>
>> Thanks,
>> Bengt
>>
>>>
>>> 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