Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Aleksey Shipilev]

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

[Claes Redestad]

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

[Aleksey Shipilev]

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

[Claes Redestad]

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

[Aleksey Shipilev]

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

[Claes Redestad]

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

[Aleksey Shipilev]

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

[Claes Redestad]

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

[Aleksey Shipilev]

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

[Remi Forax]

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" <claes.redes...@oracle.com>
> À: "Aleksey Shipilev" <sh...@redhat.com>, "Paul Sandoz" 
> <paul.san...@oracle.com>
> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>
> 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

[Claes Redestad]

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

[Aleksey Shipilev]

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

[Claes Redestad]

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

[Aleksey Shipilev]

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

[Aleksey Shipilev]

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

[Paul Sandoz]

> 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);
>}
> }
>