[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: denis kononenko <denis.kononenko () oracle ! com>
Date: 2015-05-21 11:50:42
Message-ID: 555DC692.5040600 () oracle ! com
[Download RAW message or body]
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.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic