Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]
On Mon, 17 Jun 2024 08:12:08 GMT, Adam Sotona wrote: >> Oussama Louati has updated the pull request incrementally with 19 additional >> commits since the last revision: >> >> - fix: fixed whitespaces >> - fix: fixed whitespaces >> - chore: used Class::descriptorString >> - remove: added removed test comments >> - remove: added removed test comments >> - remove: removed changes in tests >> - update: optimize imports and remove unnecessary utils >> - update: optimize imports >> - update: 5th patch of code review >> - update: 5th patch of code review >> - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7 > > test/jdk/java/lang/invoke/indify/Indify.java line 197: > >> 195: try { >> 196: runApplication(avl.toArray(new String[0])); >> 197: } catch (Exception ex) { > > Why the comments have been removed? Detailed explanations are above in those options variables declaration. Comments returned - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1646069101
Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]
On Wed, 22 May 2024 22:37:35 GMT, Oussama Louati wrote: >> test/jdk/java/lang/invoke/CallSiteTest.java line 96: >> >>> 94: } >>> 95: >>> 96: public static class MyCCS extends ConstantCallSite { >> >> Any reason for this change? > > Otherwise, it throws an java.lang.IllegalAccessError: > >> Caused by: java.lang.IllegalAccessError: failed to access class >> test.java.lang.invoke.CallSiteTest$MyCCS from class >> test.java.lang.invoke.CallSiteTest (test.java.lang.invoke.CallSiteTest$MyCCS >> is in unnamed module of loader 'app'; test.java.lang.invoke.CallSiteTest is >> in unnamed module of loader indify.Indify$Loader @234ba0f8) removed - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613824020
Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]
On Wed, 22 May 2024 08:57:23 GMT, Adam Sotona wrote: >> Oussama Louati has updated the pull request incrementally with 19 additional >> commits since the last revision: >> >> - fix: fixed whitespaces >> - fix: fixed whitespaces >> - chore: used Class::descriptorString >> - remove: added removed test comments >> - remove: added removed test comments >> - remove: removed changes in tests >> - update: optimize imports and remove unnecessary utils >> - update: optimize imports >> - update: 5th patch of code review >> - update: 5th patch of code review >> - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7 > > test/jdk/java/lang/invoke/CallSiteTest.java line 96: > >> 94: } >> 95: >> 96: public static class MyCCS extends ConstantCallSite { > > Any reason for this change? Otherwise, it throws an java.lang.IllegalAccessError: > Caused by: java.lang.IllegalAccessError: failed to access class > test.java.lang.invoke.CallSiteTest$MyCCS from class > test.java.lang.invoke.CallSiteTest (test.java.lang.invoke.CallSiteTest$MyCCS > is in unnamed module of loader 'app'; test.java.lang.invoke.CallSiteTest is > in unnamed module of loader indify.Indify$Loader @234ba0f8) > test/jdk/java/lang/invoke/CallStaticInitOrder.java line 54: > >> 52: >> 53: static int Init1Tick; >> 54: public static class Init1 { > > Is there a reason to change all the classes and methods to public? the test throws a java.lang.IllegalAccessError > test/jdk/java/lang/invoke/indify/Indify.java line 61: > >> 59: * meaning. >> 60: * >> 61: * > > Why is the javadoc reformatted? > It is not possible to review the changes if has changed. Okay i removed it > test/jdk/java/lang/invoke/indify/Indify.java line 271: > >> 269: switch (a) { >> 270: case "--java": >> 271: return; > > Where this conditional code disappeared? I wrote it back inside the writeNewClassFile() method. > test/jdk/java/lang/invoke/indify/Indify.java line 352: > >> 350: if (verbose) System.err.println("reading " + f); >> 351: ClassModel model = parseClassFile(f); >> 352: if (model == null) throw new IOException("Failed to parse class >> file: " + f.getName()); > > How it suppose to happen the model is null? I added this check to address the NullPointerException on MicroBenchmarks, it will be removed. > test/jdk/java/lang/invoke/indify/Indify.java line 354: > >> 352: if (model == null) throw new IOException("Failed to parse class >> file: " + f.getName()); >> 353: Logic logic = new Logic(model); >> 354: if(logic.classModel == null) throw new IOException("Failed to >> create logic for class file: " + f.getName()); > > And null here again? I added this check to address the NullPointerException on MicroBenchmarks, it will be removed. > test/jdk/java/lang/invoke/indify/Indify.java line 383: > >> 381: if (verbose) System.err.println("writing "+destFile); >> 382: Files.write(destFile.toPath(), new_bytes); >> 383: System.err.println("Wrote New ClassFile to: "+destFile); > > This code assumes `dest` is always specified? > For me it looks like it diverges from the original behavior. I added dest verification. > test/jdk/java/lang/invoke/indify/Indify.java line 438: > >> 436: } catch (Exception ex) { >> 437: // pass error from reportPatternMethods, etc. >> 438: throw (RuntimeException) ex; > > Casting all remaining exceptions to `RuntimeException` ? > I would suggest to keep the original code > or include `RuntimeException` in the fall through section above and wrap all > remaining exceptions into a `RuntimeException` to replicate the original > behavior. I assumed that the condition 'ex instanceof RuntimeException' is always 'true', Reverting to the original code to keep the behavior is applied. > test/jdk/java/lang/invoke/indify/Indify.java line 470: > >> 468: Logic logic = new Logic(model); >> 469: Boolean isChanged = logic.transform(); >> 470: if(!isChanged) throw new IOException("No transformation >> has been done"); > > Throwing an exception when there is not change also significantly changes the > original behavior. Removed, Thank you for pointing that out. it solved the issue of making inner classes in tests public > test/jdk/java/lang/invoke/indify/Indify.java line 503: > >> 501: >> 502: Iterator instructionIterator >> =getInstructions(m).iterator(); >> 503: final Stack shouldProceedAfterIndyAdded = new >> Stack<>(); > > What this stack of booleans suppose to do? We need a boolean value to determine if we should proceed after replacing the appropriate "pop" instruction with an "invokedynamic" instruction. However, instead of using just a boolean field, we use a stack. The reason for this is that within the lambda expression, we can
Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]
On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati wrote: >> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code >> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, >> MethodType, and CallSite constants. >> It currently uses ad-hoc code to process class files and intends to migrate >> to ASM; but since we have the Classfile API, we can migrate to Classfile API >> instead. > > Oussama Louati has updated the pull request incrementally with 19 additional > commits since the last revision: > > - fix: fixed whitespaces > - fix: fixed whitespaces > - chore: used Class::descriptorString > - remove: added removed test comments > - remove: added removed test comments > - remove: removed changes in tests > - update: optimize imports and remove unnecessary utils > - update: optimize imports > - update: 5th patch of code review > - update: 5th patch of code review > - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7 Addressing the second round of review from @asotona. - PR Review: https://git.openjdk.org/jdk/pull/18841#pullrequestreview-2076617737
Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]
On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati wrote: >> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code >> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, >> MethodType, and CallSite constants. >> It currently uses ad-hoc code to process class files and intends to migrate >> to ASM; but since we have the Classfile API, we can migrate to Classfile API >> instead. > > Oussama Louati has updated the pull request incrementally with 19 additional > commits since the last revision: > > - fix: fixed whitespaces > - fix: fixed whitespaces > - chore: used Class::descriptorString > - remove: added removed test comments > - remove: added removed test comments > - remove: removed changes in tests > - update: optimize imports and remove unnecessary utils > - update: optimize imports > - update: 5th patch of code review > - update: 5th patch of code review > - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7 I would recommend to use more high-level models of the Class-File API instead of filling the original code from a new source. There are also some unnecessary reformats of the javadoc, which is very hard to review and forgotten debug prints. For details see my inline comments. Generally it is a move forward. Changes requested by asotona (Reviewer). test/jdk/java/lang/invoke/CallSiteTest.java line 96: > 94: } > 95: > 96: public static class MyCCS extends ConstantCallSite { Any reason for this change? test/jdk/java/lang/invoke/CallStaticInitOrder.java line 54: > 52: > 53: static int Init1Tick; > 54: public static class Init1 { Is there a reason to change all the classes and methods to public? test/jdk/java/lang/invoke/indify/Indify.java line 61: > 59: * meaning. > 60: * > 61: * Why is the javadoc reformatted? It is not possible to review the changes if has changed. test/jdk/java/lang/invoke/indify/Indify.java line 271: > 269: switch (a) { > 270: case "--java": > 271: return; Where this conditional code disappeared? test/jdk/java/lang/invoke/indify/Indify.java line 352: > 350: if (verbose) System.err.println("reading " + f); > 351: ClassModel model = parseClassFile(f); > 352: if (model == null) throw new IOException("Failed to parse class > file: " + f.getName()); How it suppose to happen the model is null? test/jdk/java/lang/invoke/indify/Indify.java line 354: > 352: if (model == null) throw new IOException("Failed to parse class > file: " + f.getName()); > 353: Logic logic = new Logic(model); > 354: if(logic.classModel == null) throw new IOException("Failed to > create logic for class file: " + f.getName()); And null here again? test/jdk/java/lang/invoke/indify/Indify.java line 383: > 381: if (verbose) System.err.println("writing "+destFile); > 382: Files.write(destFile.toPath(), new_bytes); > 383: System.err.println("Wrote New ClassFile to: "+destFile); This code assumes `dest` is always specified? For me it looks like it diverges from the original behavior. test/jdk/java/lang/invoke/indify/Indify.java line 438: > 436: } catch (Exception ex) { > 437: // pass error from reportPatternMethods, etc. > 438: throw (RuntimeException) ex; Casting all remaining exceptions to `RuntimeException` ? I would suggest to keep the original code or include `RuntimeException` in the fall through section above and wrap all remaining exceptions into a `RuntimeException` to replicate the original behavior. test/jdk/java/lang/invoke/indify/Indify.java line 470: > 468: Logic logic = new Logic(model); > 469: Boolean isChanged = logic.transform(); > 470: if(!isChanged) throw new IOException("No transformation has > been done"); Throwing an exception when there is not change also significantly changes the original behavior. test/jdk/java/lang/invoke/indify/Indify.java line 503: > 501: > 502: Iterator instructionIterator > =getInstructions(m).iterator(); > 503: final Stack shouldProceedAfterIndyAdded = new > Stack<>(); What this stack of booleans suppose to do? test/jdk/java/lang/invoke/indify/Indify.java line 540: > 538: assert (i.sizeInBytes() == 3 || i2.sizeInBytes() > == 3); > 539: > System.err.println("Transforming > Method INDY Instructions & Creating New > ClassModels--}}}"); > 540: if (!quiet) System.err.println(":::Transfmoring > the Method: "+ m.methodName() +" instruction: " + i + " invokedynamic: " + > con.index() ); This debug print should be probably removed before integration.
Re: RFR: 8307818: Convert Indify tool to Classfile API [v9]
> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code > private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, > MethodType, and CallSite constants. > It currently uses ad-hoc code to process class files and intends to migrate > to ASM; but since we have the Classfile API, we can migrate to Classfile API > instead. Oussama Louati has updated the pull request incrementally with 19 additional commits since the last revision: - fix: fixed whitespaces - fix: fixed whitespaces - chore: used Class::descriptorString - remove: added removed test comments - remove: added removed test comments - remove: removed changes in tests - update: optimize imports and remove unnecessary utils - update: optimize imports - update: 5th patch of code review - update: 5th patch of code review - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7 - Changes: - all: https://git.openjdk.org/jdk/pull/18841/files - new: https://git.openjdk.org/jdk/pull/18841/files/781c951d..2b8c94a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18841=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18841=07-08 Stats: 585 lines in 6 files changed: 49 ins; 338 del; 198 mod Patch: https://git.openjdk.org/jdk/pull/18841.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18841/head:pull/18841 PR: https://git.openjdk.org/jdk/pull/18841