review request for 6798511/6860431: Include functionality of Surrogate in Character

[Ulf Zibis]

Very descriptive visualization.  :-)

I mean:
if    ( 6932837 && (review ID of 1735166) gets fixed ) {
    existing isSupplementaryCodePoint() impl is BEST
} else if ( 6933327 gets fixed ) {
    the proposed is better
} else {
    proposed is still little better than existing 
isSupplementaryCodePoint() impl
}

Additionally, toUpperCaseCharArray(), codePointCountImpl(), 
String(int[], int, int) would profit from consecutive use of 
isBMPCodePoint + isSupplementaryCodePoint() or isHighSurrogate() + 
isLowSurrogate.

 > So we will only see any benefit if they "don't fix 6932837, but fix 
6933327"?
fix 6932837 wouldn't harm, but other code, using shift by 8 | 16 would 
benefit from.

-Ulf



Am 16.03.2010 22:30, schrieb Xueming Shen:
> What did you mean "Hotspot could benefit from..."
>
> Are you saying?
>
> if    ( 6932837 gets fixed ) {
>    existing isSupplementaryCodePoint() impl is better
> } else if ( 6933327 gets fixed ) {
>    the proposed is better
> } else {
>    existing isSupplementaryCodePoint() impl might still be better
> }
>
> So we will only see any benefit if they "don't fix 6932837, but fix 
> 6933327"?
>
> -Sherman
>
>
> Ulf Zibis wrote:
>> Here you can see, how HotSpot could benefit from that bit twiddling:
>>
>> I've filed some bugs against HotSpot to optimize those cases:
>> 6932837 <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6932837> 
>> - Better use unsigned jump if one of the range limits is 0
>> 6933327 <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6933327> 
>> - Use shifted addressing modes instead of shift instuctions
>>
>> -Ulf
>>
>>
>> Am 16.03.2010 21:06, schrieb Xueming Shen:
>>> Martin Buchholz wrote:
>>>> Therefore the existing implementation
>>>>>>  return codePoint>= MIN_SUPPLEMENTARY_CODE_POINT
>>>>>> &&  codePoint<= MAX_CODE_POINT;
>>>>>>
>>>>>> will almost always perform just one comparison against a constant,
>>>>>> which is hard to beat.
>>>>>>
>>>>> 1. Wondering: I think there are TWO comparisons.
>>>>> 2. Those comparisons need to load 32 bit values from machine code, 
>>>>> against
>>>>> only 8 bit values in my case.
>>>>
>>>> It's a good point.  In the machine code, shifts are likely to use
>>>> immediate values, and so will be a small win.
>>>>
>>>> int x = codePoint >>> 16;
>>>> return x != 0 && x < 0x11;
>>>>
>>>> (On modern hardware, these optimizations
>>>> are less valuable than they used to be;
>>>> ordinary integer arithmetic is almost free)
>>>>
>>>
>>> I'm not convinced if the proposed code is really better...a "small 
>>> win".
>>>
>>> Without seeing the real native machine code generated, I'm not sure
>>> if
>>>
>>>       0: iload_0       1: bipush        16
>>>       3: iushr               4: istore_1            5: iload_1       
>>>       6: ifeq          19
>>>
>>> is really better than
>>>
>>>       0: iload_0       1: ldc           #2                  // int 
>>> 65536
>>>       3: if_icmplt     16
>>>
>>>
>>> for bmp character case, especially given the existing code has 
>>> better readability and yes, shorter....
>>>
>>> Yes, shift might be able to use the immediate values, but it still 
>>> needs to handle the "operands"
>>> and it is an extra operation. The only chance the new one might be 
>>> better is that the "ifeq" is
>>> faster than "if_icmplt", but have not worked on the instruction set 
>>> level for too long, so I can't
>>> tell (kinda remember you have to check the "circles" of each 
>>> operation to see which one is
>>> "faster" during my old gcc compiler day)
>>>
>>> OK, convince me:-)
>>>
>>> -Sherman
>>>
>>>
>>> public class Character extends java.lang.Object {
>>>  public static final int MIN_SUPPLEMENTARY_CODE_POINT = 65536;
>>>
>>>  public static final int MAX_CODE_POINT = 1114111;
>>>
>>>  public Character();
>>>    Code:
>>>       0: aload_0             1: invokespecial #1                  // 
>>> Method java/lang/Object."<init>":()V
>>>       4: return        public static boolean 
>>> isSupplementaryCodePoint(int);
>>>    Code:
>>>       0: iload_0             1: ldc           #2                  // 
>>> int 65536
>>>       3: if_icmplt     16
>>>       6: iload_0             7: ldc           #3                  // 
>>> int 1114111
>>>       9: if_icmpgt     16
>>>      12: iconst_1           13: goto          17
>>>      16: iconst_0           17: ireturn       public static boolean 
>>> isSupplementaryCodePoint_new(int);
>>>    Code:
>>>       0: iload_0             1: bipush        16
>>>       3: iushr               4: istore_1            5: iload_1       
>>>       6: ifeq          19
>>>       9: iload_1            10: bipush        17
>>>      12: if_icmpge     19
>>>      15: iconst_1           16: goto          20
>>>      19: iconst_0           20: ireturn       }
>>>
>>>
>
>