RFR: 8196869: Optimize Locale creation

[Peter Levart]

Hi Claes,

Maybe I was to quick with my clicking on Send button... If the Key 
simply held strong references to individual String attributes, 
LocaleObjectCache.cleanStaleEntries would also have to be modified to 
make sure it does not remove valid entries that happen to share equal 
Key(s) with cleared entries. So instead of this:

     private void cleanStaleEntries() {
         CacheEntry<K, V> entry;
         while ((entry = (CacheEntry<K, V>)queue.poll()) != null) {
             map.remove(entry.getKey());
         }
     }

The method would have to be like this:

     private void cleanStaleEntries() {
         CacheEntry<K, V> entry;
         while ((entry = (CacheEntry<K, V>)queue.poll()) != null) {
             map.remove(entry.getKey(), entry);
         }
     }

(Notice the use of two-argument Map.remove() method in the modified 
variant).

Regards, Peter

P.S. I now understand the hypothetical need to have individual String 
attributes wrapped with SoftReference(s) in pre-patched Key. The code 
maybe relied on the fact that SoftReference(s) to individual String 
attributes were cleared together with CacheEntry(s). When they were 
cleared, such Keys suddenly only matched themselves (i.e. no other Key 
instance would be equal to them). But if Key's SoftReference(s) were not 
cleared before corresponding CacheEntry was cleared, cleanStaleEntries() 
running concurrently with get() could remove freshly inserted entries 
too. This would not be observed as wrong behavior though. Just 
sub-optimal performance.

On 02/07/2018 01:12 PM, Peter Levart wrote:
> Hi Claes,
>
> I studied the code briefly and understand why BaseLocale.Key now has 
> to hold a SoftReference to a BaseLocale object when the same object is 
> also part of CacheEntry which is also a SoftReference. But I don't see 
> a reason why pre-patch BaseLocale.Key had to hold SoftReference(s) to 
> individual String attributes. Couldn't it simply hold strong 
> references to individual String attributes instead? The 
> LocaleObjectCache.cleanStaleEntryies() would remove cleared 
> CacheEntry(s) together with corresponding Key(s) in that case too. So 
> one SoftReference less, do you agree?
>
> I don't know if it is important for LocaleObjectCache.get() to always 
> return a canonicalized instance per key so that this always holds:
>
>     (cache.get(k1) == cache.get(k2)) == k1.equals(k2)
>
> If it is important, then I noticed a pre-existing race that violates 
> above invariant:
>
>   67             CacheEntry<K, V> newEntry = new CacheEntry<>(key, 
> newVal, queue);
>   68
>   69             entry = map.putIfAbsent(key, newEntry);
>   70             if (entry == null) {
>   71                 value = newVal;
>   72             } else {
>   73                 value = entry.get();
>   74                 if (value == null) {
>   75                     map.put(key, newEntry);
>   76                     value = newVal;
>   77                 }
>   78             }
>
> ...which can simply be fixed:
>
>             CacheEntry<K, V> newEntry = new CacheEntry<>(key, newVal, 
> queue);
>
>             while (true) {
>                 entry = map.putIfAbsent(key, newEntry);
>                 if (entry == null) {
>                     value = newVal;
>                     break;
>                 } else {
>                     value = entry.get();
>                     if (value == null) {
>                         if (map.replace(key, entry, newEntry)) {
>                             value = newVal;
>                             break;
>                         }
>                     }
>                 }
>             }
>
>
> Regards, Peter
>
>
> On 02/07/2018 11:26 AM, Claes Redestad wrote:
>> Hi Paul,
>>
>>
>> On 2018-02-06 20:55, Paul Sandoz wrote:
>>> Quick observation:
>>>
>>>   261         private BaseLocale getBaseLocale() {
>>>   262             return (holder == null) ? holderRef.get() : holder;
>>>   263         }
>>>
>>> This method can return null if the soft ref has been cleared.
>>>
>>>
>>> But you don’t check in equals:
>>>
>>>   270             if (obj instanceof Key && this.hash == 
>>> ((Key)obj).hash) {
>>>   271                 BaseLocale other = ((Key) obj).getBaseLocale();
>>>   272                 BaseLocale locale = this.getBaseLocale();
>>>   273                 if 
>>> (LocaleUtils.caseIgnoreMatch(other.getLanguage(), locale.getLanguage())
>>
>> good eye!
>>
>> It seems this wasn't caught by the existing regression tests since 
>> none of them
>> recreate Locales in that are likely to have been reclaimed, but still 
>> likely to still
>> be in the CHM (it's a race of sorts since they'll be removed when the 
>> ReferenceQueue
>> processing happen).
>>
>> I added a regression test with the smallest and quickest reproducer I 
>> could come up
>> with that provokes a NPE if we don't check null along with the fix to 
>> Key#equals:
>>
>> http://cr.openjdk.java.net/~redestad/8196869/jdk.01/
>>
>> For the normalize(Key) case we can deduce that a !normalized Key will 
>> always have
>> a strongly referenced BaseLocale and thus not need to deal with 
>> getBaseLocale()
>> returning null. I clarified this in the code and added an assert 
>> (that would be triggered
>> by the added test if it wasn't true).
>>
>> Thanks!
>>
>> /Claes
>