On Wed, 22 May 2024 08:57:23 GMT, Adam Sotona <asot...@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
