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

[Martin Buchholz]

On Tue, Mar 16, 2010 at 13:06, Xueming Shen <Xueming.Shen at sun.com> wrote:
> 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".

The primary theory here is that branches are expensive,
and we are reducing them by one.

> 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....

The very latest version of the code is Ulf's readable and optimal
(as long as it is inlined)

    public static boolean isSupplementaryCodePoint(int codePoint) {
         return !isBMPCodePoint(codePoint) && isValidCodePoint(codePoint);
    }

Martin

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