Re: RFR 8158131: Nashorn should not use jdk.internal.module.Modules API
On 30/05/2016 11:43, Sundararajan Athijegannathan wrote: Updated nashorn webrev: http://cr.openjdk.java.net/~sundar/8158131/nashorn/webrev.01/ * Added @link in ModuleGraphManipulator.java's javadoc * Using Context.class.getModule() in all places where nashorn module is needed * Using all caps names for static final variables - nashornModule, javaBaseModule, myModule in generated I looked over the changes and good to see it using the API to create dynamic modules. I assume folks familiar with the Nashorn code will do the detailed review. -Alan
Re: RFR 8158131: Nashorn should not use jdk.internal.module.Modules API
Updated nashorn webrev:
http://cr.openjdk.java.net/~sundar/8158131/nashorn/webrev.01/
* Added @link in ModuleGraphManipulator.java's javadoc
* Using Context.class.getModule() in all places where nashorn module is
needed
* Using all caps names for static final variables - nashornModule,
javaBaseModule, myModule in generated
-Sundar
On 5/30/2016 2:50 PM, Sundararajan Athijegannathan wrote:
> Hi,
>
> Inline replies below.
>
>
> On 5/30/2016 2:16 PM, Michael Haupt wrote:
>> Hi Sundar,
>>
>> lower-case thumbs up for both the jdk and nashorn changes, with remarks:
>>
>> == ModuleGraphManipulator.java ==
>>
>> * please add a @see or @link to the place where the bytes are read and
>> code is dynamically generated, or used, and vice versa.
>>
> Will do.
>
>> == JavaAdapterBytecodeGenerator.java ==
>>
>> +private static final Module nashornModule =
>> JavaAdapterBytecodeGenerator.class.getModule();
>>
>> This borders on the emerging discipline of Jigsaw idioms and patterns
>> ... assuming this class moves, at some point in the future, to another
>> (internal?) module. This will itch. How about having one central
>> authority saying "I represent the Nashorn module", rather than
>> implicitly assuming the class at hand will be in that module forever?
>> One line below, Object.class.getModule() looks like a waterproof
>> representative for the java.base module.
> hmm.. if this class moves out of nashorn , many things will break
> anyway! i.e., this has to be in nashorn (because of security assumptions
> for eg). I could use Context class perhaps..
>
>> +private static byte[] MODULES_READ_ADDER_BYRES;
>>
>> If it cannot be final, how about @Stable? It would help, if not in
>> terms of performance, so at least to document that this is an array
>> that will be populated *once* and never change thereafter.
>>
> hmm.. @Stable would bring another internal API dependency to nashorn.
> Would like to avoid another internal API dependency to nashorn.
>
>> +// private static final Module myModule;
>> +{
>> +FieldVisitor fv = cw.visitField(ACC_PRIVATE | ACC_FINAL |
>> ACC_STATIC,
>> +"myModule", "Ljava/lang/reflect/Module;", null, null);
>> +fv.visitEnd();
>> +}
>>
>> Suggestion: adhere to Java coding style even in generated code and
>> spell this as MY_MODULE.
> Will fix that.
>
> Thanks,
> -Sundar
>
>> Best,
>>
>> Michael
>>
>>> Am 30.05.2016 um 10:24 schrieb Sundararajan Athijegannathan
>>> >> >:
>>>
>>> Please review http://cr.openjdk.java.net/~sundar/8158131/
>>> Oracle is committed
>> to developing practices and products that help protect the environment
>>
>>
Re: RFR 8158131: Nashorn should not use jdk.internal.module.Modules API
Hi,
Inline replies below.
On 5/30/2016 2:16 PM, Michael Haupt wrote:
> Hi Sundar,
>
> lower-case thumbs up for both the jdk and nashorn changes, with remarks:
>
> == ModuleGraphManipulator.java ==
>
> * please add a @see or @link to the place where the bytes are read and
> code is dynamically generated, or used, and vice versa.
>
Will do.
> == JavaAdapterBytecodeGenerator.java ==
>
> +private static final Module nashornModule =
> JavaAdapterBytecodeGenerator.class.getModule();
>
> This borders on the emerging discipline of Jigsaw idioms and patterns
> ... assuming this class moves, at some point in the future, to another
> (internal?) module. This will itch. How about having one central
> authority saying "I represent the Nashorn module", rather than
> implicitly assuming the class at hand will be in that module forever?
> One line below, Object.class.getModule() looks like a waterproof
> representative for the java.base module.
hmm.. if this class moves out of nashorn , many things will break
anyway! i.e., this has to be in nashorn (because of security assumptions
for eg). I could use Context class perhaps..
>
> +private static byte[] MODULES_READ_ADDER_BYRES;
>
> If it cannot be final, how about @Stable? It would help, if not in
> terms of performance, so at least to document that this is an array
> that will be populated *once* and never change thereafter.
>
hmm.. @Stable would bring another internal API dependency to nashorn.
Would like to avoid another internal API dependency to nashorn.
> +// private static final Module myModule;
> +{
> +FieldVisitor fv = cw.visitField(ACC_PRIVATE | ACC_FINAL |
> ACC_STATIC,
> +"myModule", "Ljava/lang/reflect/Module;", null, null);
> +fv.visitEnd();
> +}
>
> Suggestion: adhere to Java coding style even in generated code and
> spell this as MY_MODULE.
Will fix that.
Thanks,
-Sundar
>
> Best,
>
> Michael
>
>> Am 30.05.2016 um 10:24 schrieb Sundararajan Athijegannathan
>> > >:
>>
>> Please review http://cr.openjdk.java.net/~sundar/8158131/
>> Oracle is committed
> to developing practices and products that help protect the environment
>
>
Re: RFR 8158131: Nashorn should not use jdk.internal.module.Modules API
Hi Sundar,
lower-case thumbs up for both the jdk and nashorn changes, with remarks:
== ModuleGraphManipulator.java ==
* please add a @see or @link to the place where the bytes are read and code is
dynamically generated, or used, and vice versa.
== JavaAdapterBytecodeGenerator.java ==
+private static final Module nashornModule =
JavaAdapterBytecodeGenerator.class.getModule();
This borders on the emerging discipline of Jigsaw idioms and patterns ...
assuming this class moves, at some point in the future, to another (internal?)
module. This will itch. How about having one central authority saying "I
represent the Nashorn module", rather than implicitly assuming the class at
hand will be in that module forever? One line below, Object.class.getModule()
looks like a waterproof representative for the java.base module.
+private static byte[] MODULES_READ_ADDER_BYRES;
If it cannot be final, how about @Stable? It would help, if not in terms of
performance, so at least to document that this is an array that will be
populated *once* and never change thereafter.
+// private static final Module myModule;
+{
+FieldVisitor fv = cw.visitField(ACC_PRIVATE | ACC_FINAL |
ACC_STATIC,
+"myModule", "Ljava/lang/reflect/Module;", null, null);
+fv.visitEnd();
+}
Suggestion: adhere to Java coding style even in generated code and spell this
as MY_MODULE.
Best,
Michael
> Am 30.05.2016 um 10:24 schrieb Sundararajan Athijegannathan
> :
>
> Please review http://cr.openjdk.java.net/~sundar/8158131/ for
> https://bugs.openjdk.java.net/browse/JDK-8158131
>
> This code cleanup is to avoid Nashorn's use of JDK internal class
> jdk.internal.module.Modules.
>
> With this change, nashorn uses java.lang.reflect.Layer public API to
> create single Module layers as needed. And nashorn generates/loads code
> into appropriate modules (which calls java.lang.reflect.Module.addReads,
> .addExports public APIs) to add module export/read edges as needed.
>
> -Sundar
>
--
Oracle is committed to developing
practices and products that help protect the environment
RFR 8158131: Nashorn should not use jdk.internal.module.Modules API
Please review http://cr.openjdk.java.net/~sundar/8158131/ for https://bugs.openjdk.java.net/browse/JDK-8158131 This code cleanup is to avoid Nashorn's use of JDK internal class jdk.internal.module.Modules. With this change, nashorn uses java.lang.reflect.Layer public API to create single Module layers as needed. And nashorn generates/loads code into appropriate modules (which calls java.lang.reflect.Module.addReads, .addExports public APIs) to add module export/read edges as needed. -Sundar
