RFR: 8307818: Convert Indify tool to Classfile API [v9]
Oussama Louati
duke at openjdk.org
Fri May 24 18:00:50 UTC 2024
On Wed, 22 May 2024 08:57:23 GMT, Adam Sotona <asotona at openjdk.org> 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: * </p>
>> 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<Instruction> instructionIterator =getInstructions(m).iterator();
>> 503: final Stack<Boolean> 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 only use final variables. By using a stack, we can update its value as needed, which is why this approach is chosen.
> 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.
Yes it's temporary for debugging and subject to removal when integrating
> test/jdk/java/lang/invoke/indify/Indify.java line 550:
>
>> 548: if (e instanceof InvokeInstruction && Objects.equals(a1, a2)) {
>> 549: System.err.println(">> Removing instruction invokestatic for Method: " + ((InvokeInstruction) e).name());
>> 550: b.andThen(b);
>
> What this suppose to do?
This will be removed.
I was forcing the classBuilder to chain itself effectively appending no new elements and removing the current e "CodeElement I,e the instruction".
> test/jdk/java/lang/invoke/indify/Indify.java line 553:
>
>> 551: } else if (shouldProceed.peek() && e instanceof InvokeInstruction && ((InvokeInstruction) e).method().equals(((InvokeInstruction) i2).method())) {
>> 552: System.err.println(">> Removing instruction invokevirtual for Method: " + ((InvokeInstruction) e).name());
>> 553: b.andThen(b);
>
> And this?
same thing for this.
> test/jdk/java/lang/invoke/indify/Indify.java line 572:
>
>> 570: } else {
>> 571: assert(i.sizeInBytes() == 3);
>> 572: System.err.println("----------------------------------------------------------------Transforming Method LDC Instructions & Creating New ClassModels------------------------------------------------------------------}}}");
>
> This debug print should be probably removed before integration.
Yes it's temporary for debugging and subject to removal when integrating
> test/jdk/java/lang/invoke/indify/Indify.java line 576:
>
>> 574: codeTransform = (b, e) ->{
>> 575: String a1 = null, a2 = null;
>> 576: if(e instanceof InvokeInstruction){
>
> Please use pattern matching and consolidate repeated `(e instanceof InvokeInstruction)`, followed by casting.
Okay
> test/jdk/java/lang/invoke/indify/Indify.java line 578:
>
>> 576: classTransform = ClassTransform.transformingMethodBodies(filter, codeTransform);
>> 577: classModel = of().parse(
>> 578: of().transform(classModel, classTransform));
>
> What this transform (in the middle of instructions iteration) does?
It updates the current classfile so when moving to a next action we avoid this runtime error:
> STATUS:Failed.`main' threw exception: java.lang.VerifyError: Bad type on operand stack
> test/jdk/java/lang/invoke/indify/Indify.java line 582:
>
>> 580: if(e instanceof InvokeInstruction && Objects.equals(a1, a2)){
>> 581: System.err.println(":::Transfmoring the Method: "+ m.methodName() +" instruction: invokestatic " + ((InvokeInstruction) e).type() + " to ldc: " + con.index() );
>> 582: b.constantInstruction(Opcode.LDC_W, ((LoadableConstantEntry) con).constantValue());
>
> `b.ldc((LoadableConstantEntry) con)` would do the same job, but so many blind casting is not a good pattern.
Change applied.
> test/jdk/java/lang/invoke/indify/Indify.java line 599:
>
>> 597: ClassModel removePatternMethodsAndVerify(ClassModel classModel){
>> 598:
>> 599: ClassModel newClassModel = of(StackMapsOption.GENERATE_STACK_MAPS).parse(
>
> `StackMapsOption.GENERATE_STACK_MAPS` is the default and it is not necessary to set it explicitly.
Okay, removed.
> test/jdk/java/lang/invoke/indify/Indify.java line 610:
>
>> 608: })
>> 609: );
>> 610: ClassHierarchyResolver classHierarchyResolver = classDesc -> ClassHierarchyResolver.ClassHierarchyInfo.ofInterface();
>
> It is not necessary to fake the `ClassHierarchyResolver` for the verification if the above code generates stack maps and is able to resolve all necessary classes.
Yes, I added it before because I had an exception thrown saying that the Verifier needs to know the classes used in the code. At least it needs to know if it is an interface or it super-class.
> test/jdk/java/lang/invoke/indify/Indify.java line 645:
>
>> 643: case GETSTATIC, GETFIELD, PUTSTATIC, PUTFIELD -> pops.replace("Q", type);
>> 644: default -> {
>> 645: if (!type.startsWith("("))
>
> This is very low-level of bytecode instructions parsing. Class-File API already contains high-level models of the instructions and it would be worth to use that models or instead of the tricky up and down conversions and auxiliary structures describing the instructions.
This is removed as its implementation is redundant and could be achieved easily using the API
> test/jdk/java/lang/invoke/indify/Indify.java line 660:
>
>> 658: *
>> 659: * @return true if any marks were changed, false otherwise.
>> 660: */
>
> This method does incremental analysis of the constant pool.
> With Class-File API CP model it can be done in one pass, or even in no-pass and all requests to "marks" can be answered directly.
Thank you for the feedback. I will look into the Class-File API CP model and apply your suggestion to perform the analysis in one pass or handle requests to "marks" directly.
> test/jdk/java/lang/invoke/indify/Indify.java line 678:
>
>> 676: if (!Modifier.isPrivate(m.flags().flagsMask())) continue;
>> 677: if (!Modifier.isStatic(m.flags().flagsMask())) continue;
>> 678: if(nameAndTypeMark(m.methodName().index(), m.methodType().index()) == mark) {
>
> Passing CP indexes instead of the entries (or maybe String values) is no more necessary.
> Original code didn't have a full model of the constant pool.
I agree, we should focus on passing CP entries as we can have all info we need from it
> test/jdk/java/lang/invoke/indify/Indify.java line 725:
>
>> 723: }
>> 724: if (entry instanceof IntegerEntry integerEntry) {
>> 725: clb.constantPool().intEntry(integerEntry.intValue());
>
> This whole code does effectively nothing and can be removed.
> All the entries present in the `oldClassModel` are iterated and and re-entered into the same constant pool.
Okay, it's removed.
> test/jdk/java/lang/invoke/indify/Indify.java line 809:
>
>> 807: }
>> 808:
>> 809: switch (poolEntry.tag()) {
>
> I would recommend to convert this to a switch expression:
>
> mark = switch (poolEntry) {
> case Utf8Entry utf -> nameMark(utf.stringValue());
> case NameAndTypeEntry nat -> nameAndTypeMark(nat.name(), nat.type());
>
>
> Questionable is why to pass CP indexes to the auxiliary methods (for further entries lookup by index), when the pool entries contain all the necessary information.
Changed to switch expression and removed passing CP indexes to auxiliary methods
> test/jdk/java/lang/invoke/indify/Indify.java line 956:
>
>> 954: case "double" -> 'D';
>> 955: default -> throw new InternalError(argClass.toString());
>> 956: };
>
> This code looks like composition of `ClassDesc`, I would look at `Class::descriptorString`
Thank you, it's now all replaced with:
> if (typeArg instanceof Class<?> argClass) {
buf.append(argClass.descriptorString());
}
> test/jdk/java/lang/invoke/indify/Indify.java line 1249:
>
>> 1247: } else {
>> 1248: return null;
>> 1249: }
>
> How does it happen that the argument might be a String or a StringEntry.
>
> Also instead of:
>
> if (x instanceof PoolEntry && ((PoolEntry) x).tag() == TAG_STRING) {
> utf8Entry = ((StringEntry) x).utf8();
>
> You can write:
>
> if (x instanceof StringEntry se) {
> utf8Entry = se.utf8();
Changed to use pattern matching:
the argument might be a String or a StringEntry happens because the method is being invoked with elements from the args list, which can contain different types of objects depending on the specific case being handled in the switch statement.
> test/jdk/java/lang/invoke/indify/Indify.java line 1273:
>
>> 1271: if(((con = args.get(argi)) instanceof MethodTypeEntry) || (con instanceof ClassEntry)){
>> 1272: assert con instanceof MethodTypeEntry;
>> 1273: type = ((MethodTypeEntry) con).descriptor();
>
> This is a lot of low-level stuff, where a bunch of entries are filled into a List<Object> and then identified back as entries. It is very hard to read and debug, I would suggest to organize it a type-safe way.
Done
> test/jdk/java/lang/invoke/indify/Indify.java line 1334:
>
>> 1332: if (x instanceof Float) { x = poolBuilder.floatEntry((Float) x); }
>> 1333: if (x instanceof Long) { x = poolBuilder.longEntry((Long) x); }
>> 1334: if (x instanceof Double) { x = poolBuilder.doubleEntry((Double) x); }
>
> This is a harder way, when you convert all the arguments into entries.
> `ConstantPoolBuilder::bsmEntry(DirectMethodHandleDesc methodReference, List<ConstantDesc> arguments)` would do it for you.
I agree, Done.
It's now using them as ClassDescs instead of creating them as entries manually so the API will do the rest of work
> test/jdk/java/lang/invoke/indify/Indify.java line 1368:
>
>> 1366: if (count == 0){
>> 1367: poolBuilder.utf8Entry("BootstrapMethods");
>> 1368: return specs;
>
> It is not necessary to manually insert "BootstrapMethods" entry, Class-File API generates all the stuff if you add a bootstrap method, no matter if it is the first one or just another addition.
Okay, it's removed
> test/jdk/java/lang/invoke/indify/Indify.java line 1403:
>
>> 1401: System.err.println(e.getMessage());
>> 1402: }
>> 1403: throw new IOException("Verification failed");
>
> Is there a reason to verify source class files?
not necessarily, just to check the class files are syntactically and semantically correct
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610748140
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610748495
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609759631
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613308306
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609776200
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609776328
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613309092
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613316282
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613496593
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613790639
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609777537
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610739563
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610739686
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609778215
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609778741
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613790489
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609784572
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609792441
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609798535
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1611726680
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613836491
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609827878
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610153523
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610193396
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613813353
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610244205
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610270258
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610582725
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610281807
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1610279669
More information about the core-libs-dev
mailing list