<i18n dev> RFR JDK-7071819: To support Extended Grapheme Clusters in Regex

[Xueming Shen]

Hi Peter,

Thanks for the review and suggestion.

This appears to be a better approach. I was wondering if I should go 
this way to cache
those lookup tables as well, but ...

Webrev has been updated as suggested.

Thanks!
Sherman

On 2/3/16 3:26 AM, Peter Levart wrote:
> Hi Again,
>
> I also have a comment on the implementation of the hash table and it's 
> GC-ness. You keep the string pool under a SoftReference because it is 
> the biggest object so it makes most sense to clear it when heap 
> becomes full. Other arrays are kept strongly reachable, but you 
> re-generate them nevertheless when string pool is cleared by GC. Would 
> it make sense to:
>
> - define all int[] arrays (including strPool) as final instance 
> variables of CharacterName
> - have one static field:
>
> private static SoftReference<CharacterName> refInstance;
>
> - convert initNamePool() into a private constructor.
>
> - convert public static getName/getCodePoint into public instance methods
>
> - introduce public static method:
>
> public static CharacterName getInstance() {
>     SoftReference<CharacterName> ref = refInstance;
>     CharacterName instance;
>     if (ref != null && ((instance = ref.get) != null) {
>         return instance;
>     }
>     instance = new CharacterName();
>     refInstance = new SoftReference<>(instance);
>     return instance;
> }
>
> ...in this arrangement, you don't need volatile fields or explicit 
> fences, as all instance fields can be final and JMM guarantees safe 
> publication in this case.
>
>
> Regards, Peter
>
> On 02/03/2016 12:07 PM, Peter Levart wrote:
>> Hi Sherman,
>>
>> I don't currently have any idea how to squeeze the hashtable any more 
>> further. It is already very compact in my opinion. But I noticed a 
>> data race that could result in navigating the half-initialized data 
>> structure. It is a classical unsafe publication bug. It has been 
>> present before in get(int cp) and it is present now in both 
>> getName(int cp) and getCodePoint(String name). For example:
>>
>>  151     static int getCodePoint(String name) {
>>  152         byte[] strPool = null;
>>  153         if (refStrPool == null || (strPool = refStrPool.get()) 
>> == null) {
>>  154             strPool = initNamePool();
>>  155         }
>>
>> vs.
>>
>> 111             refStrPool = new SoftReference<>(strPool);
>>
>> ...the static refStrPool field is not marked volatile.
>>
>> One way to fix this is to mark field volatile and then rearrange code 
>> in getName/getCodePoint to only read from it once by introducing a 
>> local var. The other would be to change the line 111 into something like:
>>
>> SoftReference<byte[]> rsp = new SoftReference<>(strPool);
>> unsafe.storeFence();
>> refStrPool = rsp;
>>
>> ...*and* also rearrange code in getName/getCodePoint to only read 
>> from field once by introducing a local var.
>>
>>
>> Regards, Peter
>>
>> On 02/02/2016 10:25 PM, Xueming Shen wrote:
>>> Hi,
>>>
>>> Have not heard any feedback on this one so far. I'm adding
>>> a little more to make it attractive for reviewers :-)
>>>
>>> On top of the \N now the webrev includes the proposal to add
>>> two more matchers, \X for unicode extended grapheme cluster
>>> and \b{g} for the corresponding boundary.
>>>
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-7071819
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8147531
>>> webrev: http://cr.openjdk.java.net/~sherman/8147531_7071819/webrev/
>>>
>>> Thanks!
>>> Sherman
>>>
>>> On 01/18/2016 11:52 PM, Xueming Shen wrote:
>>>> Hi,
>>>>
>>>> Please help review the change to add \N support in regex.
>>>>
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8147531
>>>> webrev: http://cr.openjdk.java.net/~sherman/8147531/webrev
>>>>
>>>> This is one of the items we were planning to address via JEP111
>>>> http://openjdk.java.net/jeps/111
>>>> https://bugs.openjdk.java.net/browse/JDK-8046101
>>>>
>>>> Some of the constructs had been added already in early release. I'm
>>>> planning to address the rest as individual rfe separately.
>>>>
>>>> Thanks,
>>>> Sherman
>>>
>>
>