RFR(M): 8241230: JFR: Extreme value testing: initial drop

[Leonid Mesnik]

Hi

I think you need to add jfr keyword for all tests.

See more comments (the file links are not in correct order):

http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/KpiLogger.java.html <http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/KpiLogger.java.html>

Does it make a sense to remove start or make it static to use
=  new KpiLogger("Entering record()");
or with more clean
= KpiLogger.startNewKpiLogger("Entering record()");
instead of
new KpiLogger().start("Entering record()");

so it Istant start is final and it is not possible to start logger twice for example. Also seems non-started logger doesn't make sense it these tests.

http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/TestUniqueStrings.java.html <http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/TestUniqueStrings.java.html>
lines 

53     private int nrOfBatches;
54     private List<TestEvent> events = new ArrayList<>();

makes sense to make variable final (for all tests).

105         AtomicLong checkSum = new AtomicLong();
&
108             .forEach(e -> checkSum.getAndAdd(e.getString("str").hashCode()));
Why do you need to use Atomic here? It seem that it is plain stream is used.
and it is better to use mapToLong(e -> e.getString("str").hashCode().sum() to get sum of all hashcodes. 
Also, I think it is a bad idea to sum hashcodes. It cause long overflow which might confuse people. You could just use sum of hashCode % 256 for example to check that they are the same.

Also I wonder if the order of events matter?

128         new Random().nextBytes(a);
Use Random from test library please. (It prints and use seeds)

http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/TestEventClasses.java.html <http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/TestEventClasses.java.html>

There are a lot of code duplication. Don't you want to move 
57     private static final int EVENTS_PER_BATCH = 1000;
59     private int nrOfBatches;
60     private List<Class> eventClasses = new ArrayList();
and 
72     public void run() throws Exception {
  73         String msg = String.format("Running test with %d batches, which is %d unique events",
  74                                     nrOfBatches, nrOfBatches * EVENTS_PER_BATCH);
  75         log(msg);
  76 
  77         setUp();
  78         try {
  79             Path r = record();
  80             verify(r);
  81         } finally {
  82             tearDown();
  83         }
  84     }
and
112     private void verify(Path recording) throws Exception {
 113         long referenceCheckSum = computeReferenceChecksum(nrOfBatches);
 114         AtomicLong checkSum = new AtomicLong();
 115         RecordingFile.readAllEvents(recording).stream().filter(e -> e.getEventType().getName().contains("TestEvent"))
 116                 .forEach(e -> checkSum.getAndAdd(e.getEventType().getName().hashCode()));
 117 
 118         if (referenceCheckSum != checkSum.get()) {
 119             throw new RuntimeException("TEST FAILED: event checksum mismatch");
 120         }
 121     

into abstract class, or some separate test class which is shared? So each test class add some details only to common scheme of record/verify test? It is mentioned as one of goal of initial drop to add some framework for JFR stress testing. 

 160     private void prepareGeneratedDir(String name) throws Exception {
 161         Path dir = Paths.get(name);
 162         if (Files.exists(dir)) {
 163             Files.list(dir).forEach(p -> p.toFile().delete());
 164         } else {
 165             Files.createDirectory(dir);
 166         }
 167     }
You could call cleanGeneratedDir in prepare. But really, it shouldn't exist in scratch. Also, probably you might want to look on the InMemoryCompiler in test lib. It should perfectly fit for you purpose. See

* <pre>
 * {@code
 * import jdk.test.lib.compiler.InMemoryJavaCompiler;
 * import jdk.test.lib.ByteClassLoader;
 *
 * class Example {
 *     public static void main(String[] args) {
 *         String className = "Foo";
 *         String sourceCode = "public class " + className + " {" +
 *                             "    public void bar() {" +
 *                             "        System.out.println("Hello from bar!");" +
 *                             "    }" +
 *                             "}";
 *         byte[] byteCode = InMemoryJavaCompiler.compile(className, sourceCode);
 *         Class fooClass = ByteClassLoader.load(className, byteCode);
 *     }
 * }
 * }
http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/TestRecordings.java.html <http://cr.openjdk.java.net/~mseledtsov/8241230.00/test/hotspot/jtreg/stress/jfr/extremevalues/TestRecordings.java.html>

Additionally to my common comments (final and coded)

92             for (int j = 0; j < 100; j++) {
Seems you want to use constant here.
 95             Thread.sleep(100);

 99         kpi.end("Recording lasted");
 100         return null;

Is there are any reason not to dump file and return path as in other tests for unification and  further investigation?

Leonid

> On Mar 19, 2020, at 1:18 PM, mikhailo.seledtsov at oracle.com wrote:
> 
> This is an initial drop for larger effort tracked by "8230571: [TESTBUG] JFR: Extreme value testing".
> The goal of 8230571 is to create stress JFR tests using extreme values, such as large number of
> unique events, large number of active recordings, large number of threads and more. See 8230571 for details.
> 
> This issue (8241230) was created for initial drop. It contains 3 tests: large number of unique JFR event types,
> large number of active recordings and large number of strings. Goals for this drop are:
>     - review and agree upon naming and location: location of tests, package name, test naming, etc.
>     - review patterns used for this testing (see comments in 8241230 for details)
>     - agree upon parameterized scaling via test properties
>     - and avoid "all at once" large drops
> 
> Once this change is integrated, more tests will follow as per plan outlined in 8241230. The tests will largely follow
> naming conventions and patterns agreed for this change.
> 
> 
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8241230
>     Webrev: http://cr.openjdk.java.net/~mseledtsov/8241230.00/
>     Testing:
>         Ran tests on Linux-x64, Win-x64 and MAC, with default stress level - All PASS
>         Running with higher stress levels: in progress
> 
> 
> Thank you,
> Misha
>