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

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Thu May 21 09:01:12 UTC 2015


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