RFR: 8196869: Optimize Locale creation
Hi, we can refactor sun.util.locale.BaseLocale+LocaleObjectCache to minimize the number of SoftReferences created in Locale::<clinit> and when looking up already defined BaseLocales inside the Locale constructor. http://cr.openjdk.java.net/~redestad/8196869/jdk.00/ This is mainly a tiny startup optimization, dropping executed bytecode during startup by a few thousand and reducing the minimum retained heap by a few Kb, but also speeds up microbenchmarks repeatedly calling the Locale constructor by ~1.25x. Tests stressing that dereferenced Locales are disposed of promptly remain happy. Thanks! /Claes
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()) Paul.
On Feb 6, 2018, at 9:51 AM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
we can refactor sun.util.locale.BaseLocale+LocaleObjectCache to minimize the number of SoftReferences created in Locale::<clinit> and when looking up already defined BaseLocales inside the Locale constructor.
http://cr.openjdk.java.net/~redestad/8196869/jdk.00/
This is mainly a tiny startup optimization, dropping executed bytecode during startup by a few thousand and reducing the minimum retained heap by a few Kb, but also speeds up microbenchmarks repeatedly calling the Locale constructor by ~1.25x. Tests stressing that dereferenced Locales are disposed of promptly remain happy.
Thanks!
/Claes
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
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
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
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.
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
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
Hi, On 2018-02-07 13:55, Peter Levart wrote:
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.
possible, but that'd be a larger change than I'm comfortable with for now. As Locale is initialized on bootstrap, a Cleaner-based impl. would mean starting an innocuous thread unconditionally, which would defeat the intent to optimize the minimal time to bootstrap the JVM. If we could tease things apart even further so that a SoftCleanable and Cleaners are only set up when initializing any non-constant Locale then I think we should contemplate this as a follow up. Updated webrev: http://cr.openjdk.java.net/~redestad/8196869/jdk.02/ - use map.remove(entry.getKey(), entry) instead of map.remove(entry.getKey()) - for most Locales, Locale$LocaleKey.exts is null, so using the BaseLocale as key directly in Locale allows us to avoid loading Locale$LocaleKey except in exceptional circumstances. - use map.replace to safely update the entry when putIfAbsent returns an object but it points to a cleared value, so that (benign) races to create new Locale objects will canonicalize /Claes
Hi Claes, On 02/07/2018 03:23 PM, Claes Redestad wrote:
Updated webrev:
http://cr.openjdk.java.net/~redestad/8196869/jdk.02/
- use map.remove(entry.getKey(), entry) instead of map.remove(entry.getKey()) - for most Locales, Locale$LocaleKey.exts is null, so using the BaseLocale as key directly in Locale allows us to avoid loading Locale$LocaleKey except in exceptional circumstances. - use map.replace to safely update the entry when putIfAbsent returns an object but it points to a cleared value, so that (benign) races to create new Locale objects will canonicalize
I don't know it it's only me, but I see an old version of LocaleObjectCache in webrev at above URL. Regards, Peter
On 2018-02-07 16:01, Peter Levart wrote:
Hi Claes,
On 02/07/2018 03:23 PM, Claes Redestad wrote:
Updated webrev:
http://cr.openjdk.java.net/~redestad/8196869/jdk.02/
- use map.remove(entry.getKey(), entry) instead of map.remove(entry.getKey()) - for most Locales, Locale$LocaleKey.exts is null, so using the BaseLocale as key directly in Locale allows us to avoid loading Locale$LocaleKey except in exceptional circumstances. - use map.replace to safely update the entry when putIfAbsent returns an object but it points to a cleared value, so that (benign) races to create new Locale objects will canonicalize
I don't know it it's only me, but I see an old version of LocaleObjectCache in webrev at above URL.
Sorry about that - the intended webrev have now replace the old on in-place. /Claes
Hi, On 2018-02-07 15:23, Claes Redestad wrote:
- use map.replace to safely update the entry when putIfAbsent returns an object but it points to a cleared value, so that (benign) races to create new Locale objects will canonicalize
turns out the implementation attempted here with map.replace was problematic, causing certain tests to fail. Seems there are subtle issues here around establishing a stable equality relationship which may or may not be easy to resolve, so I reverted back these particular changes to LocaleObjectCache from this RFE. I also did some cleanup in BaseLocale based on offline feedback from Paul: http://cr.openjdk.java.net/~redestad/8196869/jdk.03/ Thanks! /Claes
This is looking much better, definitely easier to understand. Paul.
On Feb 7, 2018, at 4:38 PM, Claes Redestad <claes.redestad@oracle.com> wrote:
Hi,
On 2018-02-07 15:23, Claes Redestad wrote:
- use map.replace to safely update the entry when putIfAbsent returns an object but it points to a cleared value, so that (benign) races to create new Locale objects will canonicalize
turns out the implementation attempted here with map.replace was problematic, causing certain tests to fail. Seems there are subtle issues here around establishing a stable equality relationship which may or may not be easy to resolve, so I reverted back these particular changes to LocaleObjectCache from this RFE.
I also did some cleanup in BaseLocale based on offline feedback from Paul:
http://cr.openjdk.java.net/~redestad/8196869/jdk.03/
Thanks!
/Claes
participants (3)
-
Claes Redestad
-
Paul Sandoz
-
Peter Levart