Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Chris, sorry for the late reply. Here's a version with the new naming scheme: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03.naming I like existing naming scheme and OBJECT/VOID/LONG/etc names are quite popular(e.g. Wrapper ASM (Opcodes) use them). Of course they are popular because these are the type names. There is no type L; it’s an object. I don’t understand why we have to use different names just because they are used in other namespaces. This is not a C define. I see 2 problems with the naming scheme you propose. (1) Wrapper, Opcodes BasicType collide in some places. If element names are the same, static import doesn't work and all usages should be disambiguated. (2) BasicType.I_TYPE corresponds to 5 primitive types. It's misleading to call it BasicType.INT (or BasicType.INTEGER) (consider Wrapper.INT vs BasicType.INT). Current naming scheme makes correspondence with JVM basic types explicit. Best regards, Vladimir Ivanov So, I'm in favor of leaving it as is. Best regards, Vladimir Ivanov On 3/26/14 12:24 AM, Christian Thalinger wrote: + enum BasicType { + L_TYPE('L', Object.class, Wrapper.OBJECT), // all reference types + I_TYPE('I', int.class,Wrapper.INT), + J_TYPE('J', long.class, Wrapper.LONG), + F_TYPE('F', float.class, Wrapper.FLOAT), + D_TYPE('D', double.class, Wrapper.DOUBLE), // all primitive types + V_TYPE('V', void.class, Wrapper.VOID);// not valid in all contexts I would suggest to not name them X_TYPE but give them meaningful names like Int, Float, Void. The enum BasicType already infers that it’s a type. If you think it’s not clear that it’s a type just use BasicType.Double in that places. On Mar 24, 2014, at 9:29 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov On 3/22/14 2:08 AM, Vladimir Ivanov wrote: John, thanks for the feedback. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02 Also moved LambdaForm.testShortenSignature() into a stand-alone unit test. Best regards, Vladimir Ivanov On 3/21/14 10:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Apr 8, 2014, at 1:53 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Thanks, Chris. I have to do one more iteration: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.05/ I have to revert changes related to BMH::reinvokerTarget. Removal of reinvokerTarget in generated concrete BMH classes introduces serious performance regression, since BMH::reinvokerTarget is much more complex than an accessor and it disturbs inlining decisions in too many places. OK, IIUC it's just reintroducing some original code back into BMH, nothing else has changed. If so +1 , lets get this pushed :-) I can now see why it might cause a perf issue if the following was used instead: @Override MethodHandle reinvokerTarget() { try { return (MethodHandle) argL(0); } catch (Throwable ex) { throw newInternalError(ex); } } Paul. [1] http://www.diffnow.com/?report=biopj
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Thanks, Chris. I have to do one more iteration: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.05/ I have to revert changes related to BMH::reinvokerTarget. Removal of reinvokerTarget in generated concrete BMH classes introduces serious performance regression, since BMH::reinvokerTarget is much more complex than an accessor and it disturbs inlining decisions in too many places. Best regards, Vladimir Ivanov On 4/5/14 3:31 AM, Christian Thalinger wrote: On Apr 3, 2014, at 9:44 PM, John Rose john.r.r...@oracle.com mailto:john.r.r...@oracle.com wrote: On Apr 3, 2014, at 6:33 PM, Christian Thalinger christian.thalin...@oracle.com mailto:christian.thalin...@oracle.com wrote: Of course they are popular because these are the type names. There is no type L; it’s an object. I don’t understand why we have to use different names just because they are used in other namespaces. This is not a C define. They stand for JVM signatures as well as basic types. The letters are signature letters. Can we move on from this? Sure. Push it. — John ___ mlvm-dev mailing list mlvm-...@openjdk.java.net mailto:mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Apr 3, 2014, at 9:44 PM, John Rose john.r.r...@oracle.com wrote: On Apr 3, 2014, at 6:33 PM, Christian Thalinger christian.thalin...@oracle.com wrote: Of course they are popular because these are the type names. There is no type L; it’s an object. I don’t understand why we have to use different names just because they are used in other namespaces. This is not a C define. They stand for JVM signatures as well as basic types. The letters are signature letters. Can we move on from this? Sure. Push it. — John ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Mar 26, 2014, at 8:01 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Here's a version with the new naming scheme: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03.naming I like existing naming scheme and OBJECT/VOID/LONG/etc names are quite popular(e.g. Wrapper ASM (Opcodes) use them). Of course they are popular because these are the type names. There is no type L; it’s an object. I don’t understand why we have to use different names just because they are used in other namespaces. This is not a C define. So, I'm in favor of leaving it as is. Best regards, Vladimir Ivanov On 3/26/14 12:24 AM, Christian Thalinger wrote: + enum BasicType { + L_TYPE('L', Object.class, Wrapper.OBJECT), // all reference types + I_TYPE('I', int.class,Wrapper.INT), + J_TYPE('J', long.class, Wrapper.LONG), + F_TYPE('F', float.class, Wrapper.FLOAT), + D_TYPE('D', double.class, Wrapper.DOUBLE), // all primitive types + V_TYPE('V', void.class, Wrapper.VOID);// not valid in all contexts I would suggest to not name them X_TYPE but give them meaningful names like Int, Float, Void. The enum BasicType already infers that it’s a type. If you think it’s not clear that it’s a type just use BasicType.Double in that places. On Mar 24, 2014, at 9:29 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov On 3/22/14 2:08 AM, Vladimir Ivanov wrote: John, thanks for the feedback. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02 Also moved LambdaForm.testShortenSignature() into a stand-alone unit test. Best regards, Vladimir Ivanov On 3/21/14 10:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Apr 3, 2014, at 6:33 PM, Christian Thalinger christian.thalin...@oracle.com wrote: Of course they are popular because these are the type names. There is no type L; it’s an object. I don’t understand why we have to use different names just because they are used in other namespaces. This is not a C define. They stand for JVM signatures as well as basic types. The letters are signature letters. Can we move on from this? — John
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Hi Vladimir, This looks good. Minor stuff below. I too prefer *_TYPE instead of Int/Float/Void etc as those are not v. friendly for static imports. Paul. LambaForm: -- private static int fixResult(int result, Name[] names) { if (result == LAST_RESULT) result = names.length - 1; // might still be void if (result = 0 names[result].type == V_TYPE) result = -1; return result; } change to result = -1 to: result = VOID_RESULT; ? Change: LambdaForm addArguments(int pos, ListClass? types) { BasicType[] basicTypes = new BasicType[types.size()]; for (int i = 0; i basicTypes.length; i++) basicTypes[i] = basicType(types.get(i)); return addArguments(pos, basicTypes); } to: LambdaForm addArguments(int pos, ListClass? types) { return addArguments(pos, basicTypes(types)); } ? Methods not used, i cannot tell which may be there for future code or are referenced indirectly: String basicTypeSignature() { //return LambdaForm.basicTypeSignature(resolvedHandle.type()); return LambdaForm.basicTypeSignature(methodType()); } void resolve() { for (Name n : names) n.resolve(); } static LambdaForm identityForm(BasicType type) { return LF_identityForm[type.ordinal()]; } static LambdaForm zeroForm(BasicType type) { return LF_zeroForm[type.ordinal()]; } BoundMethodHandle: -- Methods not used: SpeciesData extendWith(Class? type) { return extendWith(basicType(type)); } SpeciesData extendWithChar(char type) { return extendWith(basicType(type)); } static void initStatics() {} static { SpeciesData.initStatics(); } Deprecated method in ASM (there are also a few others): mv.visitMethodInsn(INVOKESPECIAL, className, init, makeSignature(types, true)); I think you need to append false as the last parameter. Unused first parameter cbmhClass: static NamedFunction[] makeNominalGetters(Class? cbmhClass, String types, NamedFunction[] nfs, MethodHandle[] getters) { InvokerByteCodeGenerator -- private int loadInsnOpcode(BasicType type) throws InternalError { int opcode; switch (type) { case I_TYPE: opcode = Opcodes.ILOAD; break; case J_TYPE: opcode = Opcodes.LLOAD; break; case F_TYPE: opcode = Opcodes.FLOAD; break; case D_TYPE: opcode = Opcodes.DLOAD; break; case L_TYPE: opcode = Opcodes.ALOAD; break; default: throw new InternalError(unknown type: + type); } return opcode; } Could just do: case I_TYPE: return Opcodes.ILOAD; etc. Same for storeInsnOpcode method. Unused? static int nfi = 0; MethodHandle -- Unused? /*non-public*/ MethodHandle bindImmediate(int pos, BasicType basicType, Object value) { // Bind an immediate value to a position in the arguments. // This means, elide the respective argument, // and replace all references to it in NamedFunction args with the specified value. // CURRENT RESTRICTIONS // * only for pos 0 and UNSAFE (position is adjusted in MHImpl to make API usable for others) assert pos == 0 basicType == L_TYPE value instanceof Unsafe; MethodType type2 = type.dropParameterTypes(pos, pos + 1); // adjustment: ignore receiver! LambdaForm form2 = form.bindImmediate(pos + 1, basicType, value); // adjust pos to form-relative pos return copyWith(type2, form2); } On Mar 24, 2014, at 5:29 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Paul, thanks for review. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.04/ Best regards, Vladimir Ivanov On 4/1/14 1:44 PM, Paul Sandoz wrote: Hi Vladimir, This looks good. Minor stuff below. I too prefer *_TYPE instead of Int/Float/Void etc as those are not v. friendly for static imports. Paul. LambaForm: -- private static int fixResult(int result, Name[] names) { if (result == LAST_RESULT) result = names.length - 1; // might still be void if (result = 0 names[result].type == V_TYPE) result = -1; return result; } change to result = -1 to: result = VOID_RESULT; ? Done. Change: LambdaForm addArguments(int pos, ListClass? types) { BasicType[] basicTypes = new BasicType[types.size()]; for (int i = 0; i basicTypes.length; i++) basicTypes[i] = basicType(types.get(i)); return addArguments(pos, basicTypes); } to: LambdaForm addArguments(int pos, ListClass? types) { return addArguments(pos, basicTypes(types)); } ? Done. Methods not used, i cannot tell which may be there for future code or are referenced indirectly: String basicTypeSignature() { //return LambdaForm.basicTypeSignature(resolvedHandle.type()); return LambdaForm.basicTypeSignature(methodType()); } void resolve() { for (Name n : names) n.resolve(); } static LambdaForm identityForm(BasicType type) { return LF_identityForm[type.ordinal()]; } static LambdaForm zeroForm(BasicType type) { return LF_zeroForm[type.ordinal()]; } Removed. BoundMethodHandle: -- Methods not used: SpeciesData extendWith(Class? type) { return extendWith(basicType(type)); } SpeciesData extendWithChar(char type) { return extendWith(basicType(type)); } static void initStatics() {} static { SpeciesData.initStatics(); } Removed. Deprecated method in ASM (there are also a few others): mv.visitMethodInsn(INVOKESPECIAL, className, init, makeSignature(types, true)); I think you need to append false as the last parameter. Fixed. Unused first parameter cbmhClass: static NamedFunction[] makeNominalGetters(Class? cbmhClass, String types, NamedFunction[] nfs, MethodHandle[] getters) { Removed. InvokerByteCodeGenerator -- private int loadInsnOpcode(BasicType type) throws InternalError { int opcode; switch (type) { case I_TYPE: opcode = Opcodes.ILOAD; break; case J_TYPE: opcode = Opcodes.LLOAD; break; case F_TYPE: opcode = Opcodes.FLOAD; break; case D_TYPE: opcode = Opcodes.DLOAD; break; case L_TYPE: opcode = Opcodes.ALOAD; break; default: throw new InternalError(unknown type: + type); } return opcode; } Could just do: case I_TYPE: return Opcodes.ILOAD; etc. Same for storeInsnOpcode method. Changed as you suggest. Unused? static int nfi = 0; Removed. MethodHandle -- Unused? /*non-public*/ MethodHandle bindImmediate(int pos, BasicType basicType, Object value) { // Bind an immediate value to a position in the arguments. // This means, elide the respective argument, // and replace all references to it in NamedFunction args with the specified value. // CURRENT RESTRICTIONS // * only for pos 0 and UNSAFE (position is adjusted in MHImpl to make API usable for others) assert pos == 0 basicType == L_TYPE value instanceof Unsafe; MethodType type2 = type.dropParameterTypes(pos, pos + 1); // adjustment: ignore receiver! LambdaForm form2 = form.bindImmediate(pos + 1, basicType, value); // adjust pos to form-relative pos return copyWith(type2, form2); } Removed. Best regards, Vladimir Ivanov On Mar 24, 2014, at 5:29 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Apr 1, 2014, at 3:57 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Paul, thanks for review. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.04/ +1 Paul.
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Thank you, Paul. Best regards, Vladimir Ivanov On 4/1/14 7:42 PM, Paul Sandoz wrote: On Apr 1, 2014, at 3:57 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Paul, thanks for review. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.04/ +1 Paul.
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Here's a version with the new naming scheme: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03.naming I like existing naming scheme and OBJECT/VOID/LONG/etc names are quite popular(e.g. Wrapper ASM (Opcodes) use them). So, I'm in favor of leaving it as is. Best regards, Vladimir Ivanov On 3/26/14 12:24 AM, Christian Thalinger wrote: + enum BasicType { + L_TYPE('L', Object.class, Wrapper.OBJECT), // all reference types + I_TYPE('I', int.class,Wrapper.INT), + J_TYPE('J', long.class, Wrapper.LONG), + F_TYPE('F', float.class, Wrapper.FLOAT), + D_TYPE('D', double.class, Wrapper.DOUBLE), // all primitive types + V_TYPE('V', void.class, Wrapper.VOID);// not valid in all contexts I would suggest to not name them X_TYPE but give them meaningful names like Int, Float, Void. The enum BasicType already infers that it’s a type. If you think it’s not clear that it’s a type just use BasicType.Double in that places. On Mar 24, 2014, at 9:29 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov On 3/22/14 2:08 AM, Vladimir Ivanov wrote: John, thanks for the feedback. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02 Also moved LambdaForm.testShortenSignature() into a stand-alone unit test. Best regards, Vladimir Ivanov On 3/21/14 10:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
+ enum BasicType { + L_TYPE('L', Object.class, Wrapper.OBJECT), // all reference types + I_TYPE('I', int.class,Wrapper.INT), + J_TYPE('J', long.class, Wrapper.LONG), + F_TYPE('F', float.class, Wrapper.FLOAT), + D_TYPE('D', double.class, Wrapper.DOUBLE), // all primitive types + V_TYPE('V', void.class, Wrapper.VOID);// not valid in all contexts I would suggest to not name them X_TYPE but give them meaningful names like Int, Float, Void. The enum BasicType already infers that it’s a type. If you think it’s not clear that it’s a type just use BasicType.Double in that places. On Mar 24, 2014, at 9:29 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov On 3/22/14 2:08 AM, Vladimir Ivanov wrote: John, thanks for the feedback. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02 Also moved LambdaForm.testShortenSignature() into a stand-alone unit test. Best regards, Vladimir Ivanov On 3/21/14 10:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Updated version: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/ - changed the way how arrays of types are created: static final BasicType[] ALL_TYPES = BasicType.values(); static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, ALL_TYPES.length-1); - added a test for BasicType (LambdaFormTest.testBasicType). Best regards, Vladimir Ivanov On 3/22/14 2:08 AM, Vladimir Ivanov wrote: John, thanks for the feedback. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02 Also moved LambdaForm.testShortenSignature() into a stand-alone unit test. Best regards, Vladimir Ivanov On 3/21/14 10:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
Chris, Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ Best regards, Vladimir Ivanov On 3/19/14 5:18 AM, Christian Thalinger wrote: On Mar 18, 2014, at 2:35 PM, John Rose john.r.r...@oracle.com mailto:john.r.r...@oracle.com wrote: On Mar 18, 2014, at 1:36 PM, Christian Thalinger christian.thalin...@oracle.com mailto:christian.thalin...@oracle.com wrote: Why are we not using an Enum instead of an untyped byte? Byte is moderately typed, in the sense (which I rely on during development) that you can't assign an int or char to a byte w/o a cast. That's why it is not just a plain int. But those values (L_TYPE etc.) are used a lot as numbers, and specifically as low-level array indexes, and also comparisons (x V_TYPE). To turn them into enums, we'd have to add lots of calls to '.ordinal()' to turn them right back to numbers. That dilutes (completely IMO) the value they have as enums to raise the level of the code. But without being strongly typed we get bugs like this one: @Override -MethodHandle bindArgument(int pos, char basicType, Object value) { +MethodHandle bindArgument(int pos, byte basicType, Object value) { // If the member needs dispatching, do so. if (pos == 0 basicType == 'L') { I’m just saying that for the sake of maintainability and correctness an Enum would be better. — John ___ mlvm-dev mailing list mlvm-...@openjdk.java.net mailto:mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE };
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On 03/21/2014 07:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ http://cr.openjdk.java.net/%7Evlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: + public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; + public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: + public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; + public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; or public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }.frozen(); because frozen() is a dynamic property. Rémi
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
John, thanks for the feedback. Updated webrev: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02 Also moved LambdaForm.testShortenSignature() into a stand-alone unit test. Best regards, Vladimir Ivanov On 3/21/14 10:54 PM, John Rose wrote: On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com mailto:vladimir.x.iva...@oracle.com wrote: Thanks for the feedback. What do you think about the following: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/ That looks nice. Strong typing; who woulda' thunk it. :-) The uses of .ordinal() are the extra cost relative to using just small integers. They seem totally reasonable in the code. I suggest either wrapping ordinal as something like id or else changing temp names like id to ord, to reinforce the use of a common name. Don't make the enum class public. And especially don't make the mutable arrays public: +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; — John P.S. That would only be safe if we had (what we don't yet) a notion of frozen arrays like: +public static final BasicType final[] ALL_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE }; +public static final BasicType final[] ARG_TYPES = { L_TYPE, I_TYPE, J_TYPE, F_TYPE, D_TYPE }; ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Mar 14, 2014, at 4:28 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: http://cr.openjdk.java.net/~vlivanov/8037210/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8037210 953 lines changed: 425 ins; 217 del; 311 mod This is a massive cleanup of JSR292 code to replace char-based description of basic types by numeric constants. Why are we not using an Enum instead of an untyped byte? Contributed-by: john.r.r...@oracle.com Testing: jdk/java/{lang/invoke,util}, vm.mlvm.testlist, nashorn, jruby Flags: -ea -esa -Xverify:all -D...COMPILE_THRESHOLD={0,30} Best regards, Vladimir Ivanov ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Mar 18, 2014, at 1:36 PM, Christian Thalinger christian.thalin...@oracle.com wrote: Why are we not using an Enum instead of an untyped byte? Byte is moderately typed, in the sense (which I rely on during development) that you can't assign an int or char to a byte w/o a cast. That's why it is not just a plain int. But those values (L_TYPE etc.) are used a lot as numbers, and specifically as low-level array indexes, and also comparisons (x V_TYPE). To turn them into enums, we'd have to add lots of calls to '.ordinal()' to turn them right back to numbers. That dilutes (completely IMO) the value they have as enums to raise the level of the code. — John
Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types
On Mar 18, 2014, at 2:35 PM, John Rose john.r.r...@oracle.com wrote: On Mar 18, 2014, at 1:36 PM, Christian Thalinger christian.thalin...@oracle.com wrote: Why are we not using an Enum instead of an untyped byte? Byte is moderately typed, in the sense (which I rely on during development) that you can't assign an int or char to a byte w/o a cast. That's why it is not just a plain int. But those values (L_TYPE etc.) are used a lot as numbers, and specifically as low-level array indexes, and also comparisons (x V_TYPE). To turn them into enums, we'd have to add lots of calls to '.ordinal()' to turn them right back to numbers. That dilutes (completely IMO) the value they have as enums to raise the level of the code. But without being strongly typed we get bugs like this one: @Override -MethodHandle bindArgument(int pos, char basicType, Object value) { +MethodHandle bindArgument(int pos, byte basicType, Object value) { // If the member needs dispatching, do so. if (pos == 0 basicType == 'L') { I’m just saying that for the sake of maintainability and correctness an Enum would be better. — John ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev