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
