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

[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       }
>>
>>