Re: RFR: 8332340: Add JavacBench as a test case for CDS [v2]
On Fri, 17 May 2024 19:45:00 GMT, Calvin Cheung wrote:
>> 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()
>
> 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)
The file was first added in the Leyden repo in 2023, so I think we should use
that as the initial copyright year.
> 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?
Removed.
> 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?
I moved it 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?
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047769
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047882
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047859
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047805
Re: RFR: 8332340: Add JavacBench as a test case for CDS [v2]
On Thu, 16 May 2024 17:44:22 GMT, Ioi Lam 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
Re: RFR: 8332340: Add JavacBench as a test case for CDS [v2]
On Thu, 16 May 2024 03:31:05 GMT, Chen Liang wrote: >> 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() > > test/lib/jdk/test/lib/StringArrayUtils.java line 42: > >> 40: } >> 41: >> 42: return list.toArray(new String[list.size()]); > > I thought we have been preferring ot use `new String[0]` for toArray calls. > Also for simplicity, we can change the implementation to: > > var list = new ArrayList<>(Arrays.asList(prefix)); > Collections.addAll(list, extra); > return list.toArray(new String[0]); > > or for performance: > > String[] ret = new String[prefix.length + extra.length]; > System.arraycopy(prefix, 0, ret, 0, prefix.length); > System.arraycopy(extra, 0, ret, prefix.length, extra.length); > return ret; Thanks for the suggestion. I used your `arraycopy` version. I also added some javadocs about the intended use of these `concat()` methods. - PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1603781994
Re: RFR: 8332340: Add JavacBench as a test case for CDS [v2]
> 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() - Changes: - all: https://git.openjdk.org/jdk/pull/19256/files - new: https://git.openjdk.org/jdk/pull/19256/files/10cf69d1..354665a3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19256&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19256&range=00-01 Stats: 38 lines in 2 files changed: 17 ins; 8 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19256/head:pull/19256 PR: https://git.openjdk.org/jdk/pull/19256
