Reuse the collator across String.prototype.localeCompare calls

android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java

@@ -17,6 +17,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.util.Locale;
 import java.util.stream.Collectors;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -60,6 +61,50 @@ public void testNumberFormatSignificantDigitsFromAsset() throws IOException {
     }
   }
 
+  @Test
+  public void testLocaleCompareFromAsset() throws IOException {
+    AssetManager assets =
+        InstrumentationRegistry.getInstrumentation().getTargetContext().getAssets();
+    InputStream is = assets.open("locale-compare.js");
+    String script =
+        new BufferedReader(new InputStreamReader(is)).lines().collect(Collectors.joining("\n"));
+    try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
+      rt.evaluateJavaScript(script);
+    }
+  }
+
+  @Test
+  public void testLocaleCompareFollowsDefaultLocale() {
+    Locale saved = Locale.getDefault();
+    Locale savedFormat = Locale.getDefault(Locale.Category.FORMAT);
+    Locale savedDisplay = Locale.getDefault(Locale.Category.DISPLAY);
+    Locale swedish = new Locale("sv", "SE");
+    try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
+      // Swedish sorts 'ä' after 'z', German before it.
+      Locale.setDefault(swedish);
+      rt.evaluateJavaScript("var sv = 'ä'.localeCompare('z');");
+      Locale.setDefault(Locale.GERMANY);
+      rt.evaluateJavaScript("var de = 'ä'.localeCompare('z');");
+      // Collators resolve against the default format locale.
+      Locale.setDefault(Locale.Category.FORMAT, swedish);
+      rt.evaluateJavaScript("var svFormat = 'ä'.localeCompare('z');");
+      // An unsupported explicit locale falls back to the default locale.
+      Locale.setDefault(Locale.GERMANY);
+      rt.evaluateJavaScript("var deFallback = 'ä'.localeCompare('z', 'zz');");
+      Locale.setDefault(swedish);
+      rt.evaluateJavaScript("var svFallback = 'ä'.localeCompare('z', 'zz');");
+      assertThat(rt.getGlobalNumberProperty("sv")).isEqualTo(1);
+      assertThat(rt.getGlobalNumberProperty("de")).isEqualTo(-1);
+      assertThat(rt.getGlobalNumberProperty("svFormat")).isEqualTo(1);
+      assertThat(rt.getGlobalNumberProperty("deFallback")).isEqualTo(-1);
+      assertThat(rt.getGlobalNumberProperty("svFallback")).isEqualTo(1);
+    } finally {
+      Locale.setDefault(saved);
+      Locale.setDefault(Locale.Category.FORMAT, savedFormat);
+      Locale.setDefault(Locale.Category.DISPLAY, savedDisplay);
+    }
+  }
+
   @Test
   public void testDateTimeFormat() {
     try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {

include/hermes/Platform/Intl/PlatformIntl.h

@@ -57,6 +57,12 @@ class Option {
     return str_;
   }
 
+  bool operator==(const Option &other) const {
+    if (kind_ != other.kind_)
+      return false;
+    return isString() ? str_ == other.str_ : num_ == other.num_;
+  }
+
  private:
   Kind kind_;
   // I am too lazy to make this a union.
@@ -122,6 +128,11 @@ class Collator : public vm::DecoratedObject::Decoration {
   Options resolvedOptions() noexcept;
 
   double compare(const std::u16string &x, const std::u16string &y) noexcept;
+
+  /// Whether this collator uses a default locale that is no longer the
+  /// platform's default. A collator created now from the same locales and
+  /// options would then resolve to a different locale.
+  bool defaultLocaleChanged() noexcept;
 };
 
 class DateTimeFormat : public vm::DecoratedObject::Decoration {

include/hermes/VM/JSLib/JSLibStorage.h

@@ -9,7 +9,11 @@
 #define HERMES_VM_JSLIB_RUNTIMECOMMONSTORAGE_H
 
 #include "hermes/VM/JSLib/DateCache.h"
+#ifdef HERMES_ENABLE_INTL
+#include "hermes/Platform/Intl/PlatformIntl.h"
+#endif
 
+#include <memory>
 #include <random>
 
 namespace hermes {
@@ -31,6 +35,16 @@ struct JSLibStorage {
 
   /// Time zone offset cache used in conversion between UTC and local time.
   LocalTimeOffsetCache localTimeOffsetCache;
+
+#ifdef HERMES_ENABLE_INTL
+  /// The collator String.prototype.localeCompare created last, with the
+  /// normalized locales and options it was created from.
+  struct {
+    std::vector<std::u16string> locales;
+    platform_intl::Options options;
+    std::unique_ptr<platform_intl::Collator> collator;
+  } localeCompareCollator;
+#endif
 };
 
 } // namespace vm

lib/Platform/Intl/PlatformIntlAndroid.cpp

@@ -316,6 +316,12 @@ class JCollator : public jni::JavaClass<JCollator> {
                 jni::alias_ref<jstring>, jni::alias_ref<jstring>)>("compare");
     return method(self(), x, y);
   }
+
+  bool defaultLocaleChanged() {
+    static const auto method =
+        javaClassStatic()->getMethod<jboolean()>("defaultLocaleChanged");
+    return method(self());
+  }
 };
 
 class CollatorAndroid : public Collator {
@@ -338,6 +344,10 @@ class CollatorAndroid : public Collator {
     return jCollator_->compare(stringToJava(x), stringToJava(y));
   }
 
+  bool defaultLocaleChanged() noexcept {
+    return jCollator_->defaultLocaleChanged();
+  }
+
  private:
   jni::global_ref<JCollator> jCollator_;
 };
@@ -398,6 +408,10 @@ double Collator::compare(
   return static_cast<CollatorAndroid *>(this)->compare(x, y);
 }
 
+bool Collator::defaultLocaleChanged() noexcept {
+  return static_cast<CollatorAndroid *>(this)->defaultLocaleChanged();
+}
+
 namespace {
 
 class JDateTimeFormat : public jni::JavaClass<JDateTimeFormat> {

lib/Platform/Intl/PlatformIntlApple.mm

@@ -1263,6 +1263,11 @@ uint8_t getCurrencyDigits(std::u16string_view code) {
   return static_cast<CollatorApple *>(this)->compare(x, y);
 }
 
+bool Collator::defaultLocaleChanged() noexcept {
+  // getDefaultLocale() reads the default locale once per process.
+  return false;
+}
+
 namespace {
 // Implementation of
 // https://402.ecma-international.org/8.0/#datetimeformat-objects

lib/Platform/Intl/java/com/facebook/hermes/intl/Collator.java

@@ -7,13 +7,15 @@
 
 package com.facebook.hermes.intl;
 
+import android.icu.util.ULocale;
 import android.os.Build;
 import com.facebook.proguard.annotations.DoNotStrip;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 
 /**
@@ -78,6 +80,19 @@ public class Collator {
   // [[InitializedCollator]]
   private IPlatformCollator mPlatformCollatorObject;
 
+  // The default locale that locale resolution fell back to, or null if a requested locale was used.
+  private Object mDefaultLocale;
+
+  // Must read the same locale as LocaleObject.createDefault(), or defaultLocaleChanged() compares
+  // the wrong value.
+  private static Object currentDefaultLocale() {
+    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+      return ULocale.getDefault(ULocale.Category.FORMAT);
+    } else {
+      return Locale.getDefault();
+    }
+  }
+
   // https://tc39.es/ecma402/#sec-initializecollator
   private void initializeCollator(List<String> locales, Map<String, Object> options)
       throws JSRangeErrorException {
@@ -141,6 +156,7 @@ private void initializeCollator(List<String> locales, Map<String, Object> option
     HashMap<String, Object> r = LocaleResolver.resolveLocale(locales, opt, relevantExtensionKeys);
 
     mResolvedLocaleObject = (ILocaleObject<?>) JSObjects.getJavaMap(r).get("locale");
+    mDefaultLocale = JSObjects.getJavaMap(r).get("defaultLocale");
     mResolvedLocaleObjectForResolvedOptions = mResolvedLocaleObject.cloneObject();
 
     Object collation = JSObjects.Get(r, "co");
@@ -297,4 +313,11 @@ public Map<String, Object> resolvedOptions() throws JSRangeErrorException {
   public double compare(String source, String target) {
     return mPlatformCollatorObject.compare(source, target);
   }
+
+  // Whether this collator uses a default locale that is no longer the default, in which case a
+  // collator created now from the same locales and options would resolve differently.
+  @DoNotStrip
+  public boolean defaultLocaleChanged() {
+    return mDefaultLocale != null && !mDefaultLocale.equals(currentDefaultLocale());
+  }
 }

lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleMatcher.java

@@ -19,6 +19,8 @@ public class LocaleMatcher {
   public static class LocaleMatchResult {
     public ILocaleObject<?> matchedLocale;
     public HashMap<String, String> extensions = new HashMap<>();
+    // The default locale the match fell back to because no requested locale is available, or null.
+    public Object defaultLocale;
   }
 
   // https://tc39.es/ecma402/#sec-bestavailablelocale
@@ -62,6 +64,7 @@ public static LocaleMatchResult lookupMatch(String[] requestedLocales, String[]
     }
 
     result.matchedLocale = LocaleObject.createDefault();
+    result.defaultLocale = result.matchedLocale.getLocale();
     return result;
   }
 
@@ -145,6 +148,7 @@ public static LocaleMatchResult bestFitMatch(String[] requestedLocales)
     }
 
     result.matchedLocale = LocaleObjectICU.createDefault();
+    result.defaultLocale = result.matchedLocale.getLocale();
     return result;
   }
 

lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleResolver.java

@@ -105,6 +105,7 @@ public static HashMap<String, Object> resolveLocale(
     }
 
     result.put("locale", localeMatchResult.matchedLocale);
+    result.put("defaultLocale", localeMatchResult.defaultLocale);
 
     return result;
   }

lib/VM/JSLib/Intl.cpp

@@ -13,6 +13,7 @@
 
 #include "hermes/VM/ArrayLike.h"
 #include "hermes/VM/JSLib/DateUtil.h"
+#include "hermes/VM/JSLib/JSLibStorage.h"
 #include "hermes/VM/PrimitiveBox.h"
 #include "hermes/VM/Runtime.h"
 #include "hermes/VM/StackFrame-inline.h"
@@ -1672,14 +1673,25 @@ CallResult<HermesValue> intlStringPrototypeLocaleCompare(
     return ExecutionStatus::EXCEPTION;
   }
 
-  CallResult<std::unique_ptr<platform_intl::Collator>> collatorRes =
-      platform_intl::Collator::create(runtime, *localesRes, *optionsRes);
-  if (LLVM_UNLIKELY(collatorRes == ExecutionStatus::EXCEPTION)) {
-    return ExecutionStatus::EXCEPTION;
+  // Reuse the previous collator when its inputs are equal: creating one costs
+  // far more than a comparison. This must stay after every read and conversion
+  // of the arguments, so the reuse is not observable.
+  auto &cached = runtime.getJSLibStorage()->localeCompareCollator;
+  if (!cached.collator || cached.locales != *localesRes ||
+      cached.options != *optionsRes ||
+      cached.collator->defaultLocaleChanged()) {
+    CallResult<std::unique_ptr<platform_intl::Collator>> collatorRes =
+        platform_intl::Collator::create(runtime, *localesRes, *optionsRes);
+    if (LLVM_UNLIKELY(collatorRes == ExecutionStatus::EXCEPTION)) {
+      return ExecutionStatus::EXCEPTION;
+    }
+    cached.collator = std::move(*collatorRes);
+    cached.locales = std::move(*localesRes);
+    cached.options = std::move(*optionsRes);
   }
 
   return HermesValue::encodeTrustedNumberValue(
-      (*collatorRes)->compare(*thisRes, *thatRes));
+      cached.collator->compare(*thisRes, *thatRes));
 }
 
 CallResult<HermesValue> intlStringPrototypeToLocaleLowerCase(

test/hermes/intl/locale-compare.js

@@ -0,0 +1,86 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes %s
+// REQUIRES: intl
+
+// String.prototype.localeCompare reuses its collator while the locales and
+// options stay the same. A call with other arguments must not get the
+// previous call's collator.
+
+function assertEq(actual, expected, what) {
+  if (actual !== expected) {
+    throw new Error(what + ': expected ' + expected + ', got ' + actual);
+  }
+}
+
+function sorted(locales, options) {
+  return ['Z', 'a', 'z', 'ä']
+    .sort((x, y) => x.localeCompare(y, locales, options))
+    .join();
+}
+
+for (var i = 0; i < 2; i++) {
+  assertEq(sorted('de'), 'a,ä,z,Z', 'de');
+  assertEq(sorted('sv'), 'a,z,Z,ä', 'sv');
+  assertEq(sorted('sv', {caseFirst: 'upper'}), 'a,Z,z,ä', 'sv, upper first');
+  assertEq(sorted('de', {caseFirst: 'upper'}), 'a,ä,Z,z', 'de, upper first');
+  assertEq(
+    'test10'.localeCompare('test2', 'en', {numeric: true}),
+    1,
+    'numeric',
+  );
+  assertEq('test10'.localeCompare('test2', 'en'), -1, 'not numeric');
+}
+
+// The options are still read on every call, even when the collator is reused.
+var reads = 0;
+var counted = {
+  get sensitivity() {
+    reads++;
+    return 'base';
+  },
+};
+for (var i = 0; i < 3; i++) {
+  assertEq('a'.localeCompare('A', 'en', counted), 0, 'counted getter');
+}
+assertEq(reads, 3, 'sensitivity reads');
+
+// A throwing getter throws with a warm cache, and the next call still works.
+var threw = false;
+try {
+  'a'.localeCompare('A', 'en', {
+    get sensitivity() {
+      throw new Error('boom');
+    },
+  });
+} catch (e) {
+  threw = e.message === 'boom';
+}
+assertEq(threw, true, 'throwing getter');
+assertEq('a'.localeCompare('A', 'en', counted), 0, 'after throwing getter');
+
+// A getter that calls localeCompare with other arguments does not disturb the
+// outer call.
+var reentrant = {
+  get sensitivity() {
+    assertEq(sorted('sv'), 'a,z,Z,ä', 'inner sv');
+    return 'base';
+  },
+};
+assertEq('a'.localeCompare('A', 'de', reentrant), 0, 'outer de');
+assertEq('ä'.localeCompare('z', 'de', reentrant), -1, 'outer de after inner');
+
+// An invalid option throws and leaves later calls correct.
+threw = false;
+try {
+  'a'.localeCompare('b', 'en', {usage: 'order'});
+} catch (e) {
+  threw = e instanceof RangeError;
+}
+assertEq(threw, true, 'invalid usage');
+assertEq(sorted('de'), 'a,ä,z,Z', 'de after invalid usage');