Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/24/2017 12:31 AM, Claes Redestad wrote: > http://cr.openjdk.java.net/~redestad/8186500/jdk.04/ Looks good! -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 2017-08-23 18:35, Aleksey Shipilev wrote: Sure, mind if I defer that to a future RFE, though?:-) Oh, c'mon, that should be a simple change:) And it makes the patch (that we would have to backport some day) more readable! Ok then: http://cr.openjdk.java.net/~redestad/8186500/jdk.04/ /Claes
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/23/2017 06:31 PM, Claes Redestad wrote: > > > On 08/23/2017 06:31 PM, Aleksey Shipilev wrote: >> On 08/23/2017 06:26 PM, Claes Redestad wrote: >>> On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: 332 Object value = Objects.requireNonNull(cnst); 333 if (!value.getClass().isPrimitive()) { 334 this.value = String.valueOf(cnst); 335 } else { 336 this.value = value; 337 } ...so that value is always String after this? >>> Right. Which works, and I'm not sure we really lose much this way. Nothing >>> on the >>> code generated by javac, only theoretical performance points on other code. >>> Keep >>> it simple for now and just do String.valueOf in RecipeElement always? >> Yes, and I'd probably cascade this through the code: make >> RecipeElement.value String-typed, make >> RecipeElement.getValue() return String, remove all excess String >> conversions, voila? This also makes >> the code simpler to understand. > > Sure, mind if I defer that to a future RFE, though? :-) Oh, c'mon, that should be a simple change :) And it makes the patch (that we would have to backport some day) more readable! -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/23/2017 06:31 PM, Aleksey Shipilev wrote: On 08/23/2017 06:26 PM, Claes Redestad wrote: On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: 332 Object value = Objects.requireNonNull(cnst); 333 if (!value.getClass().isPrimitive()) { 334 this.value = String.valueOf(cnst); 335 } else { 336 this.value = value; 337 } ...so that value is always String after this? Right. Which works, and I'm not sure we really lose much this way. Nothing on the code generated by javac, only theoretical performance points on other code. Keep it simple for now and just do String.valueOf in RecipeElement always? Yes, and I'd probably cascade this through the code: make RecipeElement.value String-typed, make RecipeElement.getValue() return String, remove all excess String conversions, voila? This also makes the code simpler to understand. Sure, mind if I defer that to a future RFE, though? :-) /Claes
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/23/2017 06:26 PM, Claes Redestad wrote: > On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: >> 332 Object value = Objects.requireNonNull(cnst); >> 333 if (!value.getClass().isPrimitive()) { >> 334 this.value = String.valueOf(cnst); >> 335 } else { >> 336 this.value = value; >> 337 } >> >> ...so that value is always String after this? > > Right. Which works, and I'm not sure we really lose much this way. Nothing on > the > code generated by javac, only theoretical performance points on other code. > Keep > it simple for now and just do String.valueOf in RecipeElement always? Yes, and I'd probably cascade this through the code: make RecipeElement.value String-typed, make RecipeElement.getValue() return String, remove all excess String conversions, voila? This also makes the code simpler to understand. Thanks, -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: On 08/23/2017 01:52 PM, Claes Redestad wrote: What I wasn't sure about is *when* the String.valueOf should happen, but as makeConcatWithConstants specify "If necessary, the factory would call toString to perform a one-time String conversion" then I think we could (should?) do this at our earliest convenience, which might even be in the RecipeElement construction: Yes, I don't see why not. http://cr.openjdk.java.net/~redestad/8186500/jdk.02/ Hm! *) Seems that due to cnst being an (autoboxed) reference, isPrimitive is always false on this path? Ouch... 332 Object value = Objects.requireNonNull(cnst); 333 if (!value.getClass().isPrimitive()) { 334 this.value = String.valueOf(cnst); 335 } else { 336 this.value = value; 337 } ...so that value is always String after this? Right. Which works, and I'm not sure we really lose much this way. Nothing on the code generated by javac, only theoretical performance points on other code. Keep it simple for now and just do String.valueOf in RecipeElement always? ...and that avoids String conversion on RecipeElement.getValue() paths completely? We could still do String.valueOf on the other end to make sure (as this is a no-op on Strings): http://cr.openjdk.java.net/~redestad/8186500/jdk.03 Thanks! /Claes
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/23/2017 01:52 PM, Claes Redestad wrote: > What I wasn't sure about is *when* the String.valueOf should happen, but as > makeConcatWithConstants specify "If necessary, the factory would call > toString to > perform a one-time String conversion" then I think we could (should?) do this > at > our earliest convenience, which might even be in the RecipeElement > construction: Yes, I don't see why not. > http://cr.openjdk.java.net/~redestad/8186500/jdk.02/ Hm! *) Seems that due to cnst being an (autoboxed) reference, isPrimitive is always false on this path? 332 Object value = Objects.requireNonNull(cnst); 333 if (!value.getClass().isPrimitive()) { 334 this.value = String.valueOf(cnst); 335 } else { 336 this.value = value; 337 } ...so that value is always String after this? ...and that avoids String conversion on RecipeElement.getValue() paths completely? *) If not, these are replaceable with String.valueOf: 937 String s = (cnst != null) ? cnst.toString() : "null"; ... 998 String s = (cnst != null) ? cnst.toString() : "null"; > Reworked the test to be easier to add constant types. Null constants are > specified > to throw NPE, so the test now checks that (this might be a needlessly strict > thing > to specify, but changing the method specification is out of scope here, I > think). Ah, good! I was not completely reckless with "null" checking after all! Thanks, -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
Right, the Wrapper.* code appears to work fine, but makeConcatWithConstants has pre-existing issues with non-primitive, non-String constants. What I wasn't sure about is *when* the String.valueOf should happen, but as makeConcatWithConstants specify "If necessary, the factory would call toString to perform a one-time String conversion" then I think we could (should?) do this at our earliest convenience, which might even be in the RecipeElement construction: http://cr.openjdk.java.net/~redestad/8186500/jdk.02/ Reworked the test to be easier to add constant types. Null constants are specified to throw NPE, so the test now checks that (this might be a needlessly strict thing to specify, but changing the method specification is out of scope here, I think). WDYT? /Claes On 08/22/2017 08:49 PM, Aleksey Shipilev wrote: On 08/22/2017 07:10 PM, Claes Redestad wrote: On 2017-08-22 18:34, Aleksey Shipilev wrote: http://cr.openjdk.java.net/~redestad/8186500/jdk.01/ Still think testing for {null, Class, MethodHandle, MethodType} would cover more interesting corner cases. Do you mean as constant arguments? And what should happen in each of these cases? As Remi says, should be equivalent to String.valueOf(). null: should be converted to "null" {Class, MH, MT}: should be equivalent to Class::toString My concern is that we have to check the new code, e.g. Wrapper.* does not barf when we pass something non-primitive, non-String there. Thanks, -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/22/2017 07:10 PM, Claes Redestad wrote: > On 2017-08-22 18:34, Aleksey Shipilev wrote: >>> http://cr.openjdk.java.net/~redestad/8186500/jdk.01/ >> Still think testing for {null, Class, MethodHandle, MethodType} would cover >> more interesting corner >> cases. > > Do you mean as constant arguments? And what should happen in each of these > cases? As Remi says, should be equivalent to String.valueOf(). null: should be converted to "null" {Class, MH, MT}: should be equivalent to Class::toString My concern is that we have to check the new code, e.g. Wrapper.* does not barf when we pass something non-primitive, non-String there. Thanks, -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
It should be equivalent of having a String initialized with the result of String.valueOf() as constant argument. Rémi - Mail original - > De: "Claes Redestad" > À: "Aleksey Shipilev" , "Paul Sandoz" > > Cc: "core-libs-dev" > Envoyé: Mardi 22 Août 2017 19:10:29 > Objet: Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws > AssertionError when recipe contains > non-String constants > On 2017-08-22 18:34, Aleksey Shipilev wrote: >>> http://cr.openjdk.java.net/~redestad/8186500/jdk.01/ >> Still think testing for {null, Class, MethodHandle, MethodType} would cover >> more >> interesting corner >> cases. > > Do you mean as constant arguments? And what should happen in each of > these cases? > > /Claes
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 2017-08-22 18:34, Aleksey Shipilev wrote: http://cr.openjdk.java.net/~redestad/8186500/jdk.01/ Still think testing for {null, Class, MethodHandle, MethodType} would cover more interesting corner cases. Do you mean as constant arguments? And what should happen in each of these cases? /Claes
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/22/2017 04:53 PM, Claes Redestad wrote: > Extending the existing StringConcatFactoryInvariants test we can easily get > most basic > combinations covered, along with some basic verification. > > This uncovered what appears to be a similar issue in the BC_SB SCF strategy, > which can be > dealt with by coercing the possibly non-String constant to String at > generation time: (Sigh) The trouble is LDC, right? 986 Object cnst = el.getValue(); 987 mv.visitLdcInsn(cnst); 988 desc = getSBAppendDesc(cnst.getClass()); Coercing to String is fine here. > http://cr.openjdk.java.net/~redestad/8186500/jdk.01/ Still think testing for {null, Class, MethodHandle, MethodType} would cover more interesting corner cases. Thanks, -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
Extending the existing StringConcatFactoryInvariants test we can easily get most basic combinations covered, along with some basic verification. This uncovered what appears to be a similar issue in the BC_SB SCF strategy, which can be dealt with by coercing the possibly non-String constant to String at generation time: http://cr.openjdk.java.net/~redestad/8186500/jdk.01/ /Claes On 08/21/2017 08:35 PM, Aleksey Shipilev wrote: On 08/21/2017 08:17 PM, Aleksey Shipilev wrote: On 08/21/2017 08:06 PM, Paul Sandoz wrote: On 21 Aug 2017, at 07:48, Claes Redestad wrote: a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants fails when providing an Integer as a constant, which appears to be due to failure to coerce boxed types to the corresponding primitive types when looking up various methods in StringConcatHelper: Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8186500 Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive counterpart, and is a (semantical) no-op for other types, e.g., String. Looks good. Perhaps a token test would be useful (maybe hard to test all code paths here without some combinator test). Please add tests. We have to at least assert that passing "null" works fine there -- I am not sure it does right now. Since we are talking non-String constants, passing java/lang/Object is a good test too. I remembered why we haven't covered this before: * \2 (Unicode point 0002): a constant. This input passed * through static bootstrap argument. This constant can be any value * representable in constant pool. If necessary, the factory would call * {@code toString} to perform a one-time String conversion. Neither Integer, nor Object are representable in constant pool, and so this bug cannot surface via JDK 9 bytecode, but only via direct SCF call. So, from some point of view, SCF may exhibit undefined behavior, including throwing up on non-CP-representable value. Using "should be the value representable in constant pool" instead of "can..." would make it stronger, but alas. Once other constant flavors are added (/me looks at Paul, Brian and John), it would become a possibility. This is why I think we need to add Integer and Object constant tests for future proofing SCF, while this is still a gray area. ...and maybe add {Class, MethodHandle, MethodType} as the tested arguments too, as per: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4 Thanks for taking care of this! -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/21/2017 08:17 PM, Aleksey Shipilev wrote: > On 08/21/2017 08:06 PM, Paul Sandoz wrote: >>> On 21 Aug 2017, at 07:48, Claes Redestad wrote: >>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants >>> fails >>> when providing an Integer as a constant, which appears to be due to failure >>> to >>> coerce boxed types to the corresponding primitive types when looking up >>> various >>> methods in StringConcatHelper: >>> >>> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500 >>> >>> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive >>> counterpart, and is a (semantical) no-op for other types, e.g., String. >> >> Looks good. Perhaps a token test would be useful (maybe hard to test all >> code paths here without some combinator test). > > Please add tests. We have to at least assert that passing "null" works fine > there -- I am not sure > it does right now. Since we are talking non-String constants, passing > java/lang/Object is a good > test too. I remembered why we haven't covered this before: * \2 (Unicode point 0002): a constant. This input passed * through static bootstrap argument. This constant can be any value * representable in constant pool. If necessary, the factory would call * {@code toString} to perform a one-time String conversion. Neither Integer, nor Object are representable in constant pool, and so this bug cannot surface via JDK 9 bytecode, but only via direct SCF call. So, from some point of view, SCF may exhibit undefined behavior, including throwing up on non-CP-representable value. Using "should be the value representable in constant pool" instead of "can..." would make it stronger, but alas. Once other constant flavors are added (/me looks at Paul, Brian and John), it would become a possibility. This is why I think we need to add Integer and Object constant tests for future proofing SCF, while this is still a gray area. ...and maybe add {Class, MethodHandle, MethodType} as the tested arguments too, as per: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4 Thanks for taking care of this! -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
On 08/21/2017 08:06 PM, Paul Sandoz wrote: >> On 21 Aug 2017, at 07:48, Claes Redestad wrote: >> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants >> fails >> when providing an Integer as a constant, which appears to be due to failure >> to >> coerce boxed types to the corresponding primitive types when looking up >> various >> methods in StringConcatHelper: >> >> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/ >> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500 >> >> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive >> counterpart, and is a (semantical) no-op for other types, e.g., String. > > Looks good. Perhaps a token test would be useful (maybe hard to test all code > paths here without some combinator test). Please add tests. We have to at least assert that passing "null" works fine there -- I am not sure it does right now. Since we are talking non-String constants, passing java/lang/Object is a good test too. -Aleksey
Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
> On 21 Aug 2017, at 07:48, Claes Redestad wrote: > > Hi, > > a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants > fails > when providing an Integer as a constant, which appears to be due to failure to > coerce boxed types to the corresponding primitive types when looking up > various > methods in StringConcatHelper: > > Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8186500 > > Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive > counterpart, and is a (semantical) no-op for other types, e.g., String. > Looks good. Perhaps a token test would be useful (maybe hard to test all code paths here without some combinator test). Paul. > Thanks! > > /Claes > > [1] > > import java.lang.invoke.*; > > public class Test { >public static void main(String ... args) throws Exception { > StringConcatFactory.makeConcatWithConstants(MethodHandles.lookup(), "name", >MethodType.methodType(String.class, String.class, String.class), > "\1\2\1", (Integer)1); >} > } >
RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants
Hi, a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants fails when providing an Integer as a constant, which appears to be due to failure to coerce boxed types to the corresponding primitive types when looking up various methods in StringConcatHelper: Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8186500 Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive counterpart, and is a (semantical) no-op for other types, e.g., String. Thanks! /Claes [1] import java.lang.invoke.*; public class Test { public static void main(String ... args) throws Exception { StringConcatFactory.makeConcatWithConstants(MethodHandles.lookup(), "name", MethodType.methodType(String.class, String.class, String.class), "\1\2\1", (Integer)1); } }