ICU-22582 Avoid synchronizing in RuleBasedBreakIterator and ULocale unless strictly necessary

See unicode-org#2775

Author: Diego1149

icu4j/main/core/src/main/java/com/ibm/icu/text/RuleBasedBreakIterator.java

@@ -19,8 +19,7 @@
 import java.io.OutputStream;
 import java.nio.ByteBuffer;
 import java.text.CharacterIterator;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.MissingResourceException;
 
 import com.ibm.icu.impl.CharacterIteration;
@@ -57,9 +56,6 @@ public class RuleBasedBreakIterator extends BreakIterator {
      */
     private RuleBasedBreakIterator() {
         fDictionaryCharCount  = 0;
-        synchronized(gAllBreakEngines) {
-            fBreakEngines = new ArrayList<>(gAllBreakEngines);
-        }
     }
 
     /**
@@ -174,9 +170,6 @@ public Object clone()  {
         if (fText != null) {
             result.fText = (CharacterIterator)(fText.clone());
         }
-        synchronized (gAllBreakEngines)  {
-            result.fBreakEngines = new ArrayList<>(gAllBreakEngines);
-        }
         result.fLookAheadMatches = new int[fRData.fFTable.fLookAheadResultsSize];
         result.fBreakCache = result.new BreakCache(fBreakCache);
         result.fDictionaryCache = result.new DictionaryCache(fDictionaryCache);
@@ -342,23 +335,20 @@ public int hashCode()
      * Lazily updated as break engines are needed, because instantiation of
      * break engines is expensive.
      *
-     * Because gAllBreakEngines can be referenced concurrently from different
-     * BreakIterator instances, all access is synchronized.
+     * Important notes:
+     * <ul>Because we don't want to add the same LanguageBreakEngine multiple times, all writes
+     *     are synchronized.
+     * <ul>Read access avoids explicit synchronization, but will end up being synchronized if
+     *     needed.
      */
-    private static final List<LanguageBreakEngine> gAllBreakEngines;
+    private static final ConcurrentLinkedQueue<LanguageBreakEngine> gAllBreakEngines;
 
     static {
         gUnhandledBreakEngine = new UnhandledBreakEngine();
-        gAllBreakEngines = new ArrayList<>();
+        gAllBreakEngines = new ConcurrentLinkedQueue<>();
         gAllBreakEngines.add(gUnhandledBreakEngine);
     }
 
-    /**
-     * List of all known break engines. Similar to gAllBreakEngines, but local to a
-     * break iterator, allowing it to be used without synchronization.
-     */
-    private List<LanguageBreakEngine> fBreakEngines;
-
     /**
      * Dump the contents of the state table and character classes for this break iterator.
      * For debugging only.
@@ -726,19 +716,18 @@ private LanguageBreakEngine getLanguageBreakEngine(int c) {
 
         // We have a dictionary character.
         // Does an already instantiated break engine handle it?
-        for (LanguageBreakEngine candidate : fBreakEngines) {
+        // First read without synchronization, which could lead to a new language
+        // break engine being added and we didn't go over it.
+        for (LanguageBreakEngine candidate : gAllBreakEngines) {
             if (candidate.handles(c)) {
                 return candidate;
             }
         }
 
         synchronized (gAllBreakEngines) {
-            // This break iterator's list of break engines didn't handle the character.
-            // Check the global list, another break iterator may have instantiated the
-            // desired engine.
+            // Another break iterator may have instantiated the desired engine.
             for (LanguageBreakEngine candidate : gAllBreakEngines) {
                 if (candidate.handles(c)) {
-                    fBreakEngines.add(candidate);
                     return candidate;
                 }
             }
@@ -791,7 +780,6 @@ private LanguageBreakEngine getLanguageBreakEngine(int c) {
 
             if (eng != null && eng != gUnhandledBreakEngine) {
                 gAllBreakEngines.add(eng);
-                fBreakEngines.add(eng);
             }
             return eng;
         }   // end synchronized(gAllBreakEngines)

icu4j/main/core/src/main/java/com/ibm/icu/util/ULocale.java

@@ -545,13 +545,13 @@ public Locale toLocale() {
     /**
      * Keep our own default ULocale.
      */
-    private static Locale defaultLocale = Locale.getDefault();
-    private static ULocale defaultULocale;
+    private static volatile ULocale defaultULocale;
 
     private static Locale[] defaultCategoryLocales = new Locale[Category.values().length];
     private static ULocale[] defaultCategoryULocales = new ULocale[Category.values().length];
 
     static {
+        Locale defaultLocale = Locale.getDefault();
         defaultULocale = forLocale(defaultLocale);
 
         if (JDKLocaleHelper.hasLocaleCategories()) {
@@ -581,34 +581,47 @@ public Locale toLocale() {
      * @stable ICU 2.8
      */
     public static ULocale getDefault() {
+        // Only synchronize if we must update the default locale.
+        ULocale currentDefaultULocale = defaultULocale;
+        if (currentDefaultULocale == null) {
+            // When Java's default locale has extensions (such as ja-JP-u-ca-japanese),
+            // Locale -> ULocale mapping requires BCP47 keyword mapping data that is currently
+            // stored in a resource bundle.
+            // If this happens during the class initialization's call to .forLocale(defaultLocale),
+            // then defaultULocale is still null until forLocale() returns.
+            // However, UResourceBundle currently requires non-null default ULocale.
+            // For now, this implementation returns ULocale.ROOT to avoid the problem.
+            // TODO: Consider moving BCP47 mapping data out of resource bundle later.
+            return ULocale.ROOT;
+        } else if (currentDefaultULocale.locale.equals(Locale.getDefault())) {
+            return currentDefaultULocale;
+        }
         synchronized (ULocale.class) {
-            if (defaultULocale == null) {
-                // When Java's default locale has extensions (such as ja-JP-u-ca-japanese),
-                // Locale -> ULocale mapping requires BCP47 keyword mapping data that is currently
-                // stored in a resource bundle. However, UResourceBundle currently requires
-                // non-null default ULocale. For now, this implementation returns ULocale.ROOT
-                // to avoid the problem.
+            Locale currentDefault = Locale.getDefault();
+            assert currentDefault != null;
 
-                // TODO: Consider moving BCP47 mapping data out of resource bundle later.
+            currentDefaultULocale = defaultULocale;
+            assert currentDefaultULocale != null;
 
-                return ULocale.ROOT;
+            if (currentDefaultULocale.locale.equals(currentDefault)) {
+                return currentDefaultULocale;
             }
-            Locale currentDefault = Locale.getDefault();
-            if (!defaultLocale.equals(currentDefault)) {
-                defaultLocale = currentDefault;
-                defaultULocale = forLocale(currentDefault);
-
-                if (!JDKLocaleHelper.hasLocaleCategories()) {
-                    // Detected Java default Locale change.
-                    // We need to update category defaults to match
-                    // Java 7's behavior on Android API level 21..23.
-                    for (Category cat : Category.values()) {
-                        int idx = cat.ordinal();
-                        defaultCategoryLocales[idx] = currentDefault;
-                        defaultCategoryULocales[idx] = forLocale(currentDefault);
-                    }
-                }            }
-            return defaultULocale;
+
+            ULocale nextULocale = forLocale(currentDefault);
+            assert nextULocale != null;
+
+            if (!JDKLocaleHelper.hasLocaleCategories()) {
+                // Detected Java default Locale change.
+                // We need to update category defaults to match
+                // Java 7's behavior on Android API level 21..23.
+                for (Category cat : Category.values()) {
+                    int idx = cat.ordinal();
+                    defaultCategoryLocales[idx] = currentDefault;
+                    defaultCategoryULocales[idx] = nextULocale;
+                }
+            }
+
+            return defaultULocale = nextULocale;
         }
     }
 
@@ -630,8 +643,7 @@ public static ULocale getDefault() {
      * @stable ICU 3.0
      */
     public static synchronized void setDefault(ULocale newLocale){
-        defaultLocale = newLocale.toLocale();
-        Locale.setDefault(defaultLocale);
+        Locale.setDefault(newLocale.toLocale());
         defaultULocale = newLocale;
         // This method also updates all category default locales
         for (Category cat : Category.values()) {
@@ -675,8 +687,7 @@ public static ULocale getDefault(Category category) {
                 // time.
 
                 Locale currentDefault = Locale.getDefault();
-                if (!defaultLocale.equals(currentDefault)) {
-                    defaultLocale = currentDefault;
+                if (!defaultULocale.locale.equals(currentDefault)) {
                     defaultULocale = forLocale(currentDefault);
 
                     for (Category cat : Category.values()) {