[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR (L): 8024319: Add regression tests on documented GC-related -XX:+Print* options
From:       Bengt Rutisson <bengt.rutisson () oracle ! com>
Date:       2015-01-26 9:34:06
Message-ID: 54C60A0E.6030805 () oracle ! com
[Download RAW message or body]


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


[prev in list] [next in list] [prev in thread] [next in thread]