Re: RFR (S) 8150180: String.value contents should be trusted

[Aleksey Shipilev]

Yes, sorry, pushing these two:
  http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.02
  http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/

-Aleksey

On 02/23/2016 05:39 PM, Claes Redestad wrote:
> Looks good to me, but I assume you mean to push
> http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.02 (which looks
> even better)?
> 
> Thanks!
> 
> /Claes
> 
> On 2016-02-23 11:24, Aleksey Shipilev wrote:
>> On 02/19/2016 05:42 PM, Vladimir Ivanov wrote:
 http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
 http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/
>>> Looks good.
>> A few more reviews, please?
>>
>> Cheers,
>> -Aleksey
>>
>>
> 

Re: RFR (S) 8150180: String.value contents should be trusted

[Claes Redestad]

Looks good to me, but I assume you mean to push 
http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.02 (which looks 
even better)?


Thanks!

/Claes

On 2016-02-23 11:24, Aleksey Shipilev wrote:

On 02/19/2016 05:42 PM, Vladimir Ivanov wrote:

http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/

Looks good.

A few more reviews, please?

Cheers,
-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[Aleksey Shipilev]

On 02/19/2016 05:42 PM, Vladimir Ivanov wrote:
>>http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
>>http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/
> Looks good.

A few more reviews, please?

Cheers,
-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[Christian Thalinger]

> On Feb 19, 2016, at 1:22 PM, Aleksey Shipilev  
> wrote:
> 
> On 02/20/2016 01:40 AM, Christian Thalinger wrote:
>>> On Feb 19, 2016, at 9:03 AM, John Rose  wrote:
>>> On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
>>> > 
>>> wrote:
 Why don’t you change the values to:
 
   static final byte LATIN1 = 1;
   static final byte UTF16  = 2;
> 
> We've been over this during Compact Strings development. The idea that
> John has below is related to our actual insights leading to 0/1 instead
> of 1/2. The best thing you can do with 1/2 is, apparently:
> 
>  int length() {
> return value.length >> (coder - 1);
>  }
> 
>  char charAt(int idx) {
> return getCharAt(idx * coder);// variant 1
> return getCharAt(idx << (coder - 1)); // variant 2
>  }
> 
> ...and you are better off not doing excess "-1" or
> non-strength-reducible multiplications in a very hot paths in String.
> 
> Anyhow, that ship had sailed, and any change in coder definitions would
> require to respin an awful lot of Compact String testing, and probably
> revalidating lots of premises in the code. This is good as a thought
> experiment, but not practical at this point in JDK 9 development.
> 
> 
>> But if coder is stable for both values the compiler can constant fold the 
>> if-statement for the shift value:
>> 
>>  int len = val.length >> (coder == LATIN1 ? 0 : 1);
>> 
>> That should produce the same code and we would avoid:
>> 
>> 143  * Constant-folding this field is handled internally in VM.
> 
> The constant-folding story is not the only story you should care about.
> 
> For most Strings, we do not know either String.value or String.coder
> statically. This particular constant-folding bit affects String
> concatenation with constants, but the solution to it cannot possibly
> regress an overwhelming case of non-constant Strings. Changing the coder
> designations *would* affect non-constant Strings.
> 
> I would guess that the comment on "coder" field throws a reader into
> thinking that @Stable is the answer to constant folding woes. But VM
> already trusts final fields in String (e.g. String.value.arraylength is
> folded, see JDK-8149813) -- as the part of TrustStaticNonFinalFields
> machinery, which hopefully some day would get exposed to other fields.
> Frozen arrays would hit the final (pun intended) nail into String.value
> folding. @Stable hack is doing that today; but that's a hack, for a very
> performance-sensitive corner in JDK.
> 
> Hopefully the rewritten comments spell it better:

That comment is better.

>  http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.02/
>  http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/
> 
> Cheers,
> -Aleksey
> 
> 

Re: RFR (S) 8150180: String.value contents should be trusted

[Aleksey Shipilev]

On 02/20/2016 01:40 AM, Christian Thalinger wrote:
>> On Feb 19, 2016, at 9:03 AM, John Rose  wrote:
>> On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
>> > 
>> wrote:
>>> Why don’t you change the values to:
>>>
>>>static final byte LATIN1 = 1;
>>>static final byte UTF16  = 2;

We've been over this during Compact Strings development. The idea that
John has below is related to our actual insights leading to 0/1 instead
of 1/2. The best thing you can do with 1/2 is, apparently:

  int length() {
 return value.length >> (coder - 1);
  }

  char charAt(int idx) {
 return getCharAt(idx * coder);// variant 1
 return getCharAt(idx << (coder - 1)); // variant 2
  }

...and you are better off not doing excess "-1" or
non-strength-reducible multiplications in a very hot paths in String.

Anyhow, that ship had sailed, and any change in coder definitions would
require to respin an awful lot of Compact String testing, and probably
revalidating lots of premises in the code. This is good as a thought
experiment, but not practical at this point in JDK 9 development.


> But if coder is stable for both values the compiler can constant fold the 
> if-statement for the shift value:
> 
>   int len = val.length >> (coder == LATIN1 ? 0 : 1);
> 
> That should produce the same code and we would avoid:
> 
> 143  * Constant-folding this field is handled internally in VM.

The constant-folding story is not the only story you should care about.

For most Strings, we do not know either String.value or String.coder
statically. This particular constant-folding bit affects String
concatenation with constants, but the solution to it cannot possibly
regress an overwhelming case of non-constant Strings. Changing the coder
designations *would* affect non-constant Strings.

I would guess that the comment on "coder" field throws a reader into
thinking that @Stable is the answer to constant folding woes. But VM
already trusts final fields in String (e.g. String.value.arraylength is
folded, see JDK-8149813) -- as the part of TrustStaticNonFinalFields
machinery, which hopefully some day would get exposed to other fields.
Frozen arrays would hit the final (pun intended) nail into String.value
folding. @Stable hack is doing that today; but that's a hack, for a very
performance-sensitive corner in JDK.

Hopefully the rewritten comments spell it better:
  http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.02/
  http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/

Cheers,
-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[John Rose]

On Feb 19, 2016, at 2:40 PM, Christian Thalinger 
 wrote:
> 
>> 
>> On Feb 19, 2016, at 9:03 AM, John Rose > > wrote:
>> 
>> On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
>> > 
>> wrote:
>>> 
>>> Why don’t you change the values to:
>>> 
>>>static final byte LATIN1 = 1;
>>>static final byte UTF16  = 2;
>> 
>> 
>> Not sure what you are asking, but here's my take on why 0,1 is the (slight) 
>> winner.
>> The values 0,1 are the log2 of the array element sizes 1,2, so they can be 
>> used with shift instructions.
>> With 1,2 they would have to be used with multiply instructions, or else 
>> tweaked back to shift inputs.
>> Most loops will speculate on the scale factor, but those loops which must 
>> work with a non-constant scale will be slightly cleaner with 0,1.
> 
> I see what you are saying:
> 
>   int len = val.length >> coder;  // assume LATIN1=0/UTF16=1;
> 
> But if coder is stable for both values the compiler can constant fold the 
> if-statement for the shift value:
> 
>   int len = val.length >> (coder == LATIN1 ? 0 : 1);
> 
> That should produce the same code and we would avoid:
> 
> 143  * Constant-folding this field is handled internally in VM.


Yes, the coder value can usually be speculated, especially if we encourage the 
JIT to pay special attention.
I'm only talking about the parts where the coder cannot be speculated upon.
— John

Re: RFR (S) 8150180: String.value contents should be trusted

[Christian Thalinger]

> On Feb 19, 2016, at 9:03 AM, John Rose  wrote:
> 
> On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
> > 
> wrote:
>> 
>> Why don’t you change the values to:
>> 
>>static final byte LATIN1 = 1;
>>static final byte UTF16  = 2;
> 
> 
> Not sure what you are asking, but here's my take on why 0,1 is the (slight) 
> winner.
> The values 0,1 are the log2 of the array element sizes 1,2, so they can be 
> used with shift instructions.
> With 1,2 they would have to be used with multiply instructions, or else 
> tweaked back to shift inputs.
> Most loops will speculate on the scale factor, but those loops which must 
> work with a non-constant scale will be slightly cleaner with 0,1.

I see what you are saying:

  int len = val.length >> coder;  // assume LATIN1=0/UTF16=1;

But if coder is stable for both values the compiler can constant fold the 
if-statement for the shift value:

  int len = val.length >> (coder == LATIN1 ? 0 : 1);

That should produce the same code and we would avoid:

143  * Constant-folding this field is handled internally in VM.

Re: RFR (S) 8150180: String.value contents should be trusted

[John Rose]

On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
 wrote:
> 
> Why don’t you change the values to:
> 
>static final byte LATIN1 = 1;
>static final byte UTF16  = 2;


Not sure what you are asking, but here's my take on why 0,1 is the (slight) 
winner.
The values 0,1 are the log2 of the array element sizes 1,2, so they can be used 
with shift instructions.
With 1,2 they would have to be used with multiply instructions, or else tweaked 
back to shift inputs.
Most loops will speculate on the scale factor, but those loops which must work 
with a non-constant scale will be slightly cleaner with 0,1.

— John

Re: RFR (S) 8150180: String.value contents should be trusted

[Christian Thalinger]

> On Feb 19, 2016, at 4:05 AM, Peter Levart  wrote:
> 
> Hi,
> 
> Just a stupid question.
> 
> Your comment in String:
> 
> 140  * @implNote Note this field is not {@link Stable}, because we want
> 141  * LATIN1 (0) coder to fold too. {@link Stable} would not allow that,
> 142  * as it reserves the default value as "not initialized" value.
> 143  * Constant-folding this field is handled internally in VM.
> 144  */
> 145 private final byte coder;

Why don’t you change the values to:

static final byte LATIN1 = 1;
static final byte UTF16  = 2;

?

> 
> 
> Couldn't @Stable final instance fields constant-fold the default value too in 
> general? I mean can't it be assumed that the final field has been set before 
> JIT kicks in?
> 
> Regards, Peter
> 
> On 02/19/2016 12:55 PM, Aleksey Shipilev wrote:
>> Hi,
>> 
>> Please review a simple performance improvement in Strings:
>>   https://bugs.openjdk.java.net/browse/JDK-8150180
>> 
>> In short, we want VM to trust constant String contents, so that
>> "Foo".charAt(0) is folded. As far as current Hotspot goes, this is only
>> achievable with @Stable -- we need to trust the array contents:
>>   http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
>> 
>> This, however, steps into the compiler bug caused by StringUTF16.getChar
>> intrinsic producing a mismatched load, that is then folded incorrectly.
>> So we instead rely on Java level folding in cases like these:
>>   http://cr.openjdk.java.net/~shade/8150180/webrev.hs.01/
>> 
>> ...and it works:
>>   http://cr.openjdk.java.net/~shade/8150180/notes.txt
>> 
>> While VM change looks like a workaround, Vladimir I. and me concluded
>> that @Stable folding code should just reject folding mismatched loads to
>> begin with. So, getChar intrinsic change is the actual fix. Vladimir I.
>> ought to add more assertions in @Stable folding code separately:
>>   https://bugs.openjdk.java.net/browse/JDK-8150186
>> 
>> Since this issue might trigger compiler bugs, I would like to push
>> through hs-comp to pass compiler nightlies.
>> 
>> Cheers,
>> -Aleksey
>> 
> 

Re: RFR (S) 8150180: String.value contents should be trusted

[Aleksey Shipilev]

On 02/19/2016 05:36 PM, Peter Levart wrote:
> Yes, I understand the general principle of @Stable design and the role
> of default value in @Stable fields. But given that:
> 
> But never mind. This is just me thinking loud.

Sure. The answer to those questions about @Stable is, as always: it
works like that; it was never supposed to do more than it does right
now; the time spent dealing with @Stable is better spent dealing with
generic problem of trusting final fields, without VM internal
annotations. @Stable paves the way in VM internals for that, but it does
not mean it should be catch-all feature.

-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[Vladimir Ivanov]

   http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
   http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/

Looks good.

Best regards,
Vladimir Ivanov

Re: RFR (S) 8150180: String.value contents should be trusted

[Peter Levart]

On 02/19/2016 03:07 PM, Aleksey Shipilev wrote:

Hi Peter,

On 02/19/2016 05:05 PM, Peter Levart wrote:

Your comment in String:

  140  * @implNote Note this field is not {@link Stable}, because we
want
  141  * LATIN1 (0) coder to fold too. {@link Stable} would not allow
that,
  142  * as it reserves the default value as "not initialized" value.
  143  * Constant-folding this field is handled internally in VM.
  144  */
  145 private final byte coder;


Couldn't @Stable final instance fields constant-fold the default value
too in general? I mean can't it be assumed that the final field has been
set before JIT kicks in?

We've been over this multiple times, actually. @Stable is not designed
to fold default value -- it is a part of its design to ignore final
fields with default values. Because @Stable is used largely for lazy
initialization, and default value is the important marker in field's
lifecycles.

Cheers,
-Aleksey



Yes, I understand the general principle of @Stable design and the role 
of default value in @Stable fields. But given that:


- instance final fields must be set in the constructor
- the reference to the constructed object can not be assigned to a 
static final field until the constructor is finished


...the compiler could assume that even the default value is the final 
field's stable value, couldn't it?


I understand that these are Java language constraints and not 
necessarily bytecode constraints. Also, while a static final field can 
only be assigned to reference an instance after the instance is fully 
constructed, this does not hold for @Stable fields in general, for example:


class Foo {
@Stable static Foo foo;

@Stable final int coder;

Foo() {
foo = this;
// outside code could already access Foo.foo.coder and constant 
fold it to 0

coder = 1;
}
}

...given that @Stable is an internal annotation, such usages could be 
advised against. It is generally bad practice to publish an object from 
within its constructor anyway.


But never mind. This is just me thinking loud.

Regards, Peter

Re: RFR (S) 8150180: String.value contents should be trusted

[Aleksey Shipilev]

On 02/19/2016 03:42 PM, Vladimir Ivanov wrote:
>>http://cr.openjdk.java.net/~shade/8150180/webrev.hs.01/
> 
> Why don't you check that index is also a constant? Otherwise, C2 can't
> constant fold the loads.
> 
> +  if (value->is_Con()) {
> +return false;
> +  }

Right. Without that, constant-value + non-constant-index also bail
unnecessarily. Actually, we should also bail only for getChar, but not
putChar:
  http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
  http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/

Cheers,
-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[Aleksey Shipilev]

Hi Peter,

On 02/19/2016 05:05 PM, Peter Levart wrote:
> Your comment in String:
> 
>  140  * @implNote Note this field is not {@link Stable}, because we
> want
>  141  * LATIN1 (0) coder to fold too. {@link Stable} would not allow
> that,
>  142  * as it reserves the default value as "not initialized" value.
>  143  * Constant-folding this field is handled internally in VM.
>  144  */
>  145 private final byte coder;
> 
> 
> Couldn't @Stable final instance fields constant-fold the default value
> too in general? I mean can't it be assumed that the final field has been
> set before JIT kicks in?

We've been over this multiple times, actually. @Stable is not designed
to fold default value -- it is a part of its design to ignore final
fields with default values. Because @Stable is used largely for lazy
initialization, and default value is the important marker in field's
lifecycles.

Cheers,
-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[Peter Levart]

Hi,

Just a stupid question.

Your comment in String:

 140  * @implNote Note this field is not {@link Stable}, because we 
want
 141  * LATIN1 (0) coder to fold too. {@link Stable} would not 
allow that,

 142  * as it reserves the default value as "not initialized" value.
 143  * Constant-folding this field is handled internally in VM.
 144  */
 145 private final byte coder;


Couldn't @Stable final instance fields constant-fold the default value 
too in general? I mean can't it be assumed that the final field has been 
set before JIT kicks in?


Regards, Peter

On 02/19/2016 12:55 PM, Aleksey Shipilev wrote:

Hi,

Please review a simple performance improvement in Strings:
   https://bugs.openjdk.java.net/browse/JDK-8150180

In short, we want VM to trust constant String contents, so that
"Foo".charAt(0) is folded. As far as current Hotspot goes, this is only
achievable with @Stable -- we need to trust the array contents:
   http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/

This, however, steps into the compiler bug caused by StringUTF16.getChar
intrinsic producing a mismatched load, that is then folded incorrectly.
So we instead rely on Java level folding in cases like these:
   http://cr.openjdk.java.net/~shade/8150180/webrev.hs.01/

...and it works:
   http://cr.openjdk.java.net/~shade/8150180/notes.txt

While VM change looks like a workaround, Vladimir I. and me concluded
that @Stable folding code should just reject folding mismatched loads to
begin with. So, getChar intrinsic change is the actual fix. Vladimir I.
ought to add more assertions in @Stable folding code separately:
   https://bugs.openjdk.java.net/browse/JDK-8150186

Since this issue might trigger compiler bugs, I would like to push
through hs-comp to pass compiler nightlies.

Cheers,
-Aleksey

Re: RFR (S) 8150180: String.value contents should be trusted

[Vladimir Ivanov]

   http://cr.openjdk.java.net/~shade/8150180/webrev.hs.01/


Why don't you check that index is also a constant? Otherwise, C2 can't 
constant fold the loads.


+  if (value->is_Con()) {
+return false;
+  }

Best regards,
Vladimir Ivanov