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
