Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Fri, 24 May 2024 17:12:34 GMT, Oussama Louati wrote: >> 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 only use final variables. By using > a stack, we can update its value as needed, which is why this approach is > chosen. I see here an iteration over instructions of a method, where the whole class is retransformed in certain situations and some status is passed back in a stack of booleans. The whole conversion should be implemented in a single transformation. Original code repeatedly replaced instructions inline (that is BTW reason why added nops below), however architecture of ClassFile API is different, you are transforming one class into completely new class (free to remove and add as many elements as you need). You can compose transformations into complex trees and you can also collect information before the transformation, however the class transformation should be executed only once. - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1642418788
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Tue, 28 May 2024 11:03:18 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 one additional > commit since the last revision: > > chore: changed pool marking initialization to be done in one pass test/jdk/java/lang/invoke/indify/Indify.java line 197: > 195: break; > 196: case "-v": case "--verbose": case "--verbose=": > 197: verbose = booleanOption(a2); // more output Why the comments have been removed? - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1642379144
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Fri, 24 May 2024 17:12:24 GMT, Oussama Louati wrote: >> 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 Injected transform to avoid verify error, that seems to me conceptually wrong. What is the root cause of the verify error and how the transform suppose to fix it? - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1642372748
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Wed, 22 May 2024 22:38:16 GMT, Oussama Louati wrote:
>> 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
Changed to original access modifier
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1624137975
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati wrote: >> 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. Done - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1617070598
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
> 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 one additional commit since the last revision: chore: changed pool marking initialization to be done in one pass - Changes: - all: https://git.openjdk.org/jdk/pull/18841/files - new: https://git.openjdk.org/jdk/pull/18841/files/2b8c94a7..0cac912d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18841=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18841=08-09 Stats: 91 lines in 1 file changed: 9 ins; 53 del; 29 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
