RFR: 8307818: Convert Indify tool to Classfile API [v9]
Adam Sotona
asotona at openjdk.org
Fri May 24 18:00:49 UTC 2024
On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati <duke at openjdk.org> 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: * </p>
> 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<Instruction> instructionIterator =getInstructions(m).iterator();
> 503: final Stack<Boolean> 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.
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?
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?
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.
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.
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?
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.
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.
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.
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.
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.
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.
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.
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.
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`
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();
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.
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.
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.
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?
-------------
Changes requested by asotona (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18841#pullrequestreview-2070398734
PR Review: https://git.openjdk.org/jdk/pull/18841#pullrequestreview-2076472008
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609572765
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609572065
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609564032
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613228454
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609557109
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609557531
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613231918
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613245509
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613247462
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613250834
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609549832
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609555023
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609555309
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609548549
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609547516
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613256739
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609541900
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609536757
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609535430
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609527200
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613260699
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609520089
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609507447
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609498352
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613263637
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609486000
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609480304
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609473262
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609468693
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609462205
More information about the core-libs-dev
mailing list