RFR: 8196869: Optimize Locale creation

[Peter Levart]

Hi Claes,

On 02/07/2018 01:48 PM, Claes Redestad wrote:
> Hi,
>
> On 2018-02-07 13:12, 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?
>
> cleanStaleEntries is sufficient for making sure the Key gets cleared 
> eventually, yes, but having
> part of the Key softly reachable expedites memory reclamation in some 
> important cases.

When no cleanStaleEntries() is called for a long time, right?

Would making CacheEntry extend jdk.internal.ref.SoftCleanable instead of 
SoftReference help here? You could remove the cleanStaleEntries method 
entirely and just remove the Map entry in SoftCleanable's performCleanup 
method.

Regards, Peter

>
>>
>> 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)
>
> I believe LocaleObjectCache is intended as a best effort cache 
> solution with soft memory
> semantics for performance, not a strict canonicalization facility. 
> That said I think removing
> the race you pointed out here is probably a good thing to do.
>
> /Claes
>
>>
>> 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
>>
>