RFR: 8172169: Re-examine String field optionality

[Claes Redestad]

Hi,

On 2017-01-02 20:36, Vladimir Kozlov wrote:
> Hi Claes,
>
> There should not be performance impact on compiled code since the check
> is done during compilation.
> And there could be a negligible effect in Interpreter.

the changes to opto will have negligible or no effect, yes, but there
are also code paths taken by GC and other code which call 
java_lang_String::is_latin1 where the coder_offset check won't be
removed.

I don't expect to see any remarkable improvements, however.

>
> I agree that coder field should be not optional since jdk 9. I think it
> was made optional at the initial development phase of compact stings
> implementation (8141132) when we had both types of String class.
>
> I also remember there were tests (and customers .jar files) which put
> different implementation of String class on boot class path which failed
> if we don't check presence of these fields. But with modules in jdk 9 it
> may not relevant any more.

Replacing String will most likely have to be done differently in 9.

>
> Removing hash field optionality is fine since looking back on 6924259
> changes - hash field was never removed from String class:
>
> http://hg.openjdk.java.net/jdk9/hs/jdk/rev/2c773daa825d
>
> http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/8f972594effc

Agree, this is what we concluded as well looking at the history here.

>
> The only concern I have is testing. You need to run as many as possible
> jdk and Hotspot tests (use RBT).

I've submitted a larger body of tests through RBT to try and see if
there are any tests I've missed.

Thanks!

/Claes

>
> Thanks,
> Vladimir
>
> On 1/2/17 8:54 AM, Claes Redestad wrote:
>> Hi,
>>
>> hash_offset and coder_offset are set up to be optional, although there is
>> code that doesn't honor this (which would be a bug if the fields actually
>> *were* optional).  When the optionality is honored, there's a performance
>> risk, especially on platforms with weak or no branch prediction.
>>
>> Since optional fields makes little sense in a world without hotspot
>> express,
>> I think these should simply be made non-optional.
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8172169/webrev.01
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8172169
>>
>> Testing: JPRT pass.
>>
>> Thanks!
>>
>> /Claes