RFR (S) 8134758: Final String field values should be trusted as stable

[Per Liden]

Hi Aleksey,

On 2015-09-01 14:31, Aleksey Shipilev wrote:
> Hi Per,
>
> Yes, I wondered how String Deduplication works with the String.value
> stability. I guessed that as long as dedup is idempotent (e.g. it
> switches the char[] to the semantically the same char[]), we are fine.
> Even if we inline the "old" reference to char[] into the generated code,
> it seems to become the GC root on its own.

We do actually have to handle this as little bit of a special case. As 
you say, it's correct that we're semantically safe if we replace one 
char[] with another char[] containing the same bytes. However, during 
the string dedup development we could see a significant performance drop 
for many applications, because the String.equals/compare intrinsic no 
longer found a match in the fast path. To avoid this we're actively 
deduplicating string before they get interned (see 
StringTable::intern()) and hence before the char[] of a String constant 
gets inlined into the code. I.e. we make sure deduplication never change 
an interned String because those char[]s can be inlined.

>
> And yes, this particular change does nothing new to that mechanics.

I must admit that I don't fully understand all implications of this 
change (if any). I just want to make sure we're not introducing some 
hard to detect performance regression (e.g. a fast path is no longer 
taken) which would need special treatment like the case I described above.

cheers,
/Per

>
> Thanks,
> -Aleksey
>
> On 09/01/2015 02:43 PM, Per Liden wrote:
>> Hi Aleksey,
>>
>> I'm trying to figure out if this has any impact on String deduplication
>> (which can change the String.value field), but I can't see any obvious
>> problems. Dedup already handles the case where GraphKit treats it as a
>> stable field and I don't see that this adds any new cases. Just wanted
>> to double check that you reached the same conclusion?
>>
>> cheers,
>> /Per
>>
>> On 2015-08-31 16:59, Aleksey Shipilev wrote:
>>> Hi,
>>>
>>> I would like to make a forward move and make VM to trust all final
>>> String fields. I cannot quickly find the scenario where it helps current
>>> JDK -- there is only String.value field, which components are not
>>> treated as constants anyway. But, it helps a lot the upcoming Compact
>>> Strings change, which introduces String.coder field.
>>>
>>> String.value is actually handled as stable in the GraphKit with
>>> UseImplicitStableValues, but it does not affect "normal" Java code.
>>> Therefore, in a way, this change extends the same behavior to the normal
>>> code. See more here:
>>>     https://bugs.openjdk.java.net/browse/JDK-8134758
>>>
>>> Here is a patch:
>>>     http://cr.openjdk.java.net/~shade/8134758/webrev.00/
>>>
>>> Passes JPRT and eyeballed assembly looks fine on Linux x86_64.
>>>
>>> Does the change itself look generic enough to consider straight in the
>>> mainline? Otherwise, we can keep it in Compact Strings sandbox, but it
>>> will eventually arrive back entangled in a much larger code change, and
>>> shall still require review.
>>>
>>> Thanks,
>>> -Aleksey
>>>
>
>