RFR: 8217442: Optimize native accesses to String.value

[David Holmes]

Hi Claes,

As per our IM discussion lets go back to your first version as both 
versions break encapsulation to a similar level. I'll send out 
additional review email for that.

Follow up below just to express an opinion ... :)

On 22/01/2019 8:56 am, Claes Redestad wrote:
> Hi,
> 
> why yes, those checks do seem necessary, don't they? So I did to some
> digging before I settled on an approach (Erik Österlund helped me
> interpret the profiles and explain to me why the repeated accesses are
> a cost in these cases).
> 
> It appears it's related to crashes when debugging Strings that have only
> been partially initialized. See
> https://bugs.openjdk.java.net/browse/JDK-8025922 and
> https://bugs.openjdk.java.net/browse/JDK-8021897 for more details.

Hmmmm - despite being involved in one of the above code reviews I'm 
highly suspect of those particular fixes. I don't think the main JNI API 
needs concern itself with partially initialized Strings - how would it 
get one? JNI doesn't do error checking so this would seem like a user 
error to me - caveat emptor. For the debugger case I think the fix was 
done at the wrong end of the debugger - why should the debugger ask the 
VM for "this" when single stepping the constructor? The debugger has to 
be be aware that it's dealing with uninitialized memory.

Anyway we won't be revisiting these decisions any time soon.

Cheers,
David

> So I don't think it's advisable to remove the null checks. And even
> without them there would be a few double-accesses left to optimize away.
> 
> Thanks!
> 
> /Claes
> 
> On 2019-01-21 23:41, David Holmes wrote:
>> Hi Claes,
>>
>> Sorry to do this to you but I'm questioning the underlying assumption 
>> here regarding the existing patterns of checking value for null before 
>> getting the length. According to String.java
>>
>>      * Additionally, it is marked with {@link Stable} to trust the 
>> contents
>>       * of the array. No other facility in JDK provides this 
>> functionality (yet).
>>       * {@link Stable} is safe here, because value is never null.
>>       */
>>      @Stable
>>      private final byte[] value;
>>
>> value can never be null so the null checks should be removeable, then 
>> you wouldn't have the double-accesses to value.
>>
>> David
>> -----
>>
>> On 22/01/2019 8:07 am, Claes Redestad wrote:
>>> Hi David,
>>>
>>> On 2019-01-21 21:54, David Holmes wrote:
>>>> What if we refactored so that
>>>>
>>>> java_lang_String::length(oop java_string, typeArrayOop value)
>>>>
>>>> became
>>>>
>>>> java_lang_String::length(typeArrayOop value, bool isLatin1)
>>>>
>>>> would that also achieve the goal while presenting (IMHO) a cleaner API?
>>>
>>> doing so would get rid of the contorted asserts at least, and seems to
>>> be performance neutral on all operations:
>>>
>>> http://cr.openjdk.java.net/~redestad/8217442/open.02/
>>>
>>> Re-running tests.
>>>
>>> /Claes