RFR: 8354329: Rewrite runtime/ClassFile/JsrRewriting.java and OomWhileParsingRepeatedJsr.java tests [v4]

[Matias Saavedra Silva]

On Wed, 30 Apr 2025 08:47:30 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> Rewrote JsrRewriting and OomWhileParsingRepeatedJsr tests so that they are not using jar files, but instead class files are created on the fly using the ASM library. 
>> 
>> Test in tiers 1-3 on multiple platforms pass.
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8354329: Got rid of unused variables.

This is a good change! I have a few comments on style but overall the implementation looks good.

test/hotspot/jtreg/runtime/ClassFile/JsrRewriting.java line 47:

> 45: import jdk.test.lib.process.OutputAnalyzer;
> 46: 
> 47: import java.io.File;

Imports should be clustered together in alphabetical order

test/hotspot/jtreg/runtime/ClassFile/OOMCrashClass1960_2.java line 33:

> 31: 
> 32: /*
> 33: This is class is a dumper for the original OOMCrashClass1960_2.class, i.e. its dump() method

Was there a copyright date on the original version of this class? If so maybe it should be included in the header

test/hotspot/jtreg/runtime/ClassFile/OOMCrashClass1960_2.java line 63:

> 61:             methodVisitor.visitMethodInsn(INVOKESTATIC, "java/lang/Integer", "parseInt", "(Ljava/lang/String;)I", false);
> 62:             methodVisitor.visitVarInsn(ISTORE, 1);
> 63:             Label label1 = new Label();

Just a suggestion so you can ignore this if you prefer the current style: Declaring labels in the middle of the code is a bit confusing since it isn't actually applied until you call `visitLabel()`. Maybe it would be better to declare the labels at the top of the method.

test/hotspot/jtreg/runtime/ClassFile/OomWhileParsingRepeatedJsr.java line 45:

> 43:  */
> 44: 
> 45: import jdk.test.lib.process.ProcessTools;

Same as above, imports should be in alphabetical order

test/hotspot/jtreg/runtime/ClassFile/OomWhileParsingRepeatedJsr.java line 69:

> 67:         // in order to trigger and observe a fake os::malloc oom. This needs NMT.
> 68:         ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder(
> 69:                 "-cp", ".",

Align the arguments here

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

Changes requested by matsaave (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24662#pullrequestreview-2812140573
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071687874
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071705626
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071702225
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071707308
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071708172