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

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8332340: Add JavacBench as a test case for CDS [v2]
From:       Calvin Cheung <ccheung () openjdk ! org>
Date:       2024-05-17 19:48:03
Message-ID: uHz0meNJLwDh09AJIl2LjOw_1OxU3h6Fk2sQCGXMmIE=.6c16cc02-4cac-4701-9b74-90301f9281a2 () github ! com
[Download RAW message or body]

On Thu, 16 May 2024 17:44:22 GMT, Ioi Lam <iklam@openjdk.org> wrote:

> > JavacBench is a test program that compiles 90 Java source files. It uses a fair \
> > amount of invokedynamic callsites, so it's good for testing CDS support for indy \
> > and lambda expressions. 
> > This test was first integrated into the \
> > [leyden](https://github.com/openjdk/leyden/tree/premain) repo. Hence some of the \
> > files have copyrights in 2023.
> 
> Ioi Lam has updated the pull request incrementally with one additional commit since \
> the last revision: 
> (1) comments from @liach; (2) added javadoc; (3) Use createTestJavaProcessBuilder() \
> instead of createLimitedTestJavaProcessBuilder()

A few minor comments.

test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBench.java line 2:

> 1: /*
> 2:  * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.

Since this is a new file, should the copyright year be only 2024?
(same for other files)

test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBench.java line 43:

> 41: import jdk.test.lib.cds.CDSAppTester;
> 42: import jdk.test.lib.helpers.ClassFileInstaller;
> 43: import jdk.test.lib.process.OutputAnalyzer;

Is this import needed?

test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBenchApp.java line 222:

> 220:         }
> 221:         long elapsed = System.currentTimeMillis() - started;
> 222:         if (System.getProperty("JavacBenchApp.silent") == null) {

Should line 221 be inside the 'if' block?

test/lib/jdk/test/lib/cds/CDSAppTester.java line 218:

> 216:         }
> 217: 
> 218:         throw new RuntimeException("Must have exactly one command line \
> argument: STATIC or DYNAMIC");

Why not check the number of args at the beginning of the method?

-------------

PR Review: https://git.openjdk.org/jdk/pull/19256#pullrequestreview-2064242660
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605479322
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605468705
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605474939
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1605477874


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