[intl] Specialize GetOption

Creates two different functions specialized for string and boolean
types.

Gets rid of several allocations and keeps the C++ <-> JS type
conversions to a minimum. Improves the API as we don't have to create
the fallback or V8 Strings unnecessarily.

Bug: v8:5751
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: I788e43e6ef23f3e9144ff719f01d6334fe6cb9ce
Reviewed-on: https://chromium-review.googlesource.com/1126750
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54259}

src/isolate.h

@@ -200,6 +200,13 @@ class WasmEngine;
     return __isolate__->Throw(*__isolate__->factory()->call); \
   } while (false)
 
+#define THROW_NEW_ERROR_RETURN_VALUE(isolate, call, value) \
+  do {                                                     \
+    Isolate* __isolate__ = (isolate);                      \
+    __isolate__->Throw(*__isolate__->factory()->call);     \
+    return value;                                          \
+  } while (false)
+
 #define RETURN_ON_EXCEPTION_VALUE(isolate, call, value)            \
   do {                                                             \
     if ((call).is_null()) {                                        \

src/objects.cc

@@ -18941,57 +18941,6 @@ MaybeHandle<Name> FunctionTemplateInfo::TryGetCachedPropertyName(
   return MaybeHandle<Name>();
 }
 
-#ifdef V8_INTL_SUPPORT
-MaybeHandle<Object> Object::GetOption(
-    Isolate* isolate, Handle<JSReceiver> options, Handle<Name> property,
-    Object::OptionType type, Handle<FixedArray> values, Handle<Object> fallback,
-    Handle<String> services) {
-  // 1. Let value be ? Get(options, property).
-  Handle<Object> value;
-  ASSIGN_RETURN_ON_EXCEPTION(
-      isolate, value, Object::GetPropertyOrElement(options, property), Object);
-
-  // 2. If value is not undefined, then
-  if (!value->IsUndefined(isolate)) {
-    // 2. b. If type is "boolean", then
-    if (type == Object::OptionType::Boolean) {
-      // 2. b. i. Let value be ToBoolean(value).
-      value = isolate->factory()->ToBoolean(value->BooleanValue(isolate));
-    }
-
-    // 2. c. If type is "string", then
-    if (type == Object::OptionType::String) {
-      // 2. c. i. Let value be ? ToString(value).
-      ASSIGN_RETURN_ON_EXCEPTION(isolate, value,
-                                 Object::ToString(isolate, value), Object);
-    }
-
-    // 2. d. if values is not undefined, then
-    if (values->length() > 0) {
-      // 2. d. i. If values does not contain an element equal to value,
-      // throw a RangeError exception.
-      for (int i = 0; i < values->length(); i++) {
-        if (value->StrictEquals(values->get(i))) {
-          // 2. e. return value
-          return value;
-        }
-      }
-
-      THROW_NEW_ERROR(isolate,
-                      NewRangeError(MessageTemplate::kValueOutOfRange, value,
-                                    services, property),
-                      Object);
-    }
-
-    // 2. e. return value
-    return value;
-  }
-
-  // 3. Else, return fallback.
-  return fallback;
-}
-#endif  // V8_INTL_SUPPORT
-
 // Force instantiation of template instances class.
 // Please note this list is compiler dependent.
 // Keep this at the end of this file

src/objects.h

@@ -1259,23 +1259,6 @@ class Object {
   // and length.
   bool IterationHasObservableEffects();
 
-  enum class OptionType : bool { String, Boolean };
-
-#ifdef V8_INTL_SUPPORT
-  // ECMA402 9.2.10. GetOption( options, property, type, type, values, fallback)
-  // ecma402/#sec-getoption
-  //
-  // Instead of passing undefined for the values argument as the spec
-  // defines, pass in an empty fixed array.
-  //
-  // service is a string denoting the type of Intl object; used when
-  // printing the error message.
-  V8_WARN_UNUSED_RESULT static MaybeHandle<Object> GetOption(
-      Isolate* isolate, Handle<JSReceiver> options, Handle<Name> property,
-      Object::OptionType type, Handle<FixedArray> values,
-      Handle<Object> fallback, Handle<String> service);
-#endif  // V8_INTL_SUPPORT
-
   DECL_VERIFIER(Object)
 #ifdef VERIFY_HEAP
   // Verify a pointer is a valid object pointer.

src/objects/intl-objects.cc

@@ -1237,5 +1237,79 @@ MaybeHandle<JSReceiver> Intl::UnwrapReceiver(Isolate* isolate,
   return Handle<JSReceiver>::cast(new_receiver);
 }
 
+Maybe<bool> Intl::GetStringOption(Isolate* isolate, Handle<JSReceiver> options,
+                                  const char* property,
+                                  std::vector<const char*> values,
+                                  const char* service,
+                                  std::unique_ptr<char[]>* result) {
+  Handle<String> property_str =
+      isolate->factory()->NewStringFromAsciiChecked(property);
+
+  // 1. Let value be ? Get(options, property).
+  Handle<Object> value;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, value, Object::GetPropertyOrElement(options, property_str),
+      Nothing<bool>());
+
+  if (value->IsUndefined(isolate)) {
+    return Just(false);
+  }
+
+  // 2. c. Let value be ? ToString(value).
+  Handle<String> value_str;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, value_str, Object::ToString(isolate, value), Nothing<bool>());
+  std::unique_ptr<char[]> value_cstr = value_str->ToCString();
+
+  // 2. d. if values is not undefined, then
+  if (values.size() > 0) {
+    // 2. d. i. If values does not contain an element equal to value,
+    // throw a RangeError exception.
+    for (size_t i = 0; i < values.size(); i++) {
+      if (strcmp(values.at(i), value_cstr.get()) == 0) {
+        // 2. e. return value
+        *result = std::move(value_cstr);
+        return Just(true);
+      }
+    }
+
+    Handle<String> service_str =
+        isolate->factory()->NewStringFromAsciiChecked(service);
+    THROW_NEW_ERROR_RETURN_VALUE(
+        isolate,
+        NewRangeError(MessageTemplate::kValueOutOfRange, value, service_str,
+                      property_str),
+        Nothing<bool>());
+  }
+
+  // 2. e. return value
+  *result = std::move(value_cstr);
+  return Just(true);
+}
+
+V8_WARN_UNUSED_RESULT Maybe<bool> Intl::GetBoolOption(
+    Isolate* isolate, Handle<JSReceiver> options, const char* property,
+    const char* service, bool* result) {
+  Handle<String> property_str =
+      isolate->factory()->NewStringFromAsciiChecked(property);
+
+  // 1. Let value be ? Get(options, property).
+  Handle<Object> value;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, value, Object::GetPropertyOrElement(options, property_str),
+      Nothing<bool>());
+
+  // 2. If value is not undefined, then
+  if (!value->IsUndefined(isolate)) {
+    // 2. b. i. Let value be ToBoolean(value).
+    *result = value->BooleanValue(isolate);
+
+    // 2. e. return value
+    return Just(true);
+  }
+
+  return Just(false);
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/intl-objects.h

@@ -206,6 +206,42 @@ class Intl {
       Handle<JSFunction> constructor, Intl::Type type,
       Handle<String> method_name /* TODO(gsathya): Make this char const* */,
       bool check_legacy_constructor = false);
+
+  // ECMA402 9.2.10. GetOption( options, property, type, values, fallback)
+  // ecma402/#sec-getoption
+  //
+  // This is specialized for the case when type is string.
+  //
+  // Instead of passing undefined for the values argument as the spec
+  // defines, pass in an empty vector.
+  //
+  // Returns true if options object has the property and stores the
+  // result in value. Returns false if the value is not found. The
+  // caller is required to use fallback value appropriately in this
+  // case.
+  //
+  // service is a string denoting the type of Intl object; used when
+  // printing the error message.
+  V8_WARN_UNUSED_RESULT static Maybe<bool> GetStringOption(
+      Isolate* isolate, Handle<JSReceiver> options, const char* property,
+      std::vector<const char*> values, const char* service,
+      std::unique_ptr<char[]>* result);
+
+  // ECMA402 9.2.10. GetOption( options, property, type, values, fallback)
+  // ecma402/#sec-getoption
+  //
+  // This is specialized for the case when type is boolean.
+  //
+  // Returns true if options object has the property and stores the
+  // result in value. Returns false if the value is not found. The
+  // caller is required to use fallback value appropriately in this
+  // case.
+  //
+  // service is a string denoting the type of Intl object; used when
+  // printing the error message.
+  V8_WARN_UNUSED_RESULT static Maybe<bool> GetBoolOption(
+      Isolate* isolate, Handle<JSReceiver> options, const char* property,
+      const char* service, bool* result);
 };
 
 }  // namespace internal

src/objects/js-locale.cc

@@ -17,6 +17,7 @@
 #include "src/heap/factory.h"
 #include "src/isolate.h"
 #include "src/objects-inl.h"
+#include "src/objects/intl-objects.h"
 #include "src/objects/js-locale-inl.h"
 #include "unicode/locid.h"
 #include "unicode/unistr.h"
@@ -46,37 +47,28 @@ Maybe<bool> InsertOptionsIntoLocale(Isolate* isolate,
                                  {"numeric", "kn"},
                                  {"numberingSystem", "nu"}}};
 
-  Handle<Object> undefined = isolate->factory()->undefined_value();
-  Handle<FixedArray> empty_array = isolate->factory()->empty_fixed_array();
-  Handle<String> service =
-      isolate->factory()->NewStringFromAsciiChecked("locale");
+  // TODO(cira): Pass in values as per the spec to make this to be
+  // spec compliant.
+  std::vector<const char*> values = {};
 
   for (const auto& option_to_bcp47 : kOptionToUnicodeTagMap) {
-    Handle<String> key_str =
-        isolate->factory()->NewStringFromAsciiChecked(option_to_bcp47.first);
-
-    Handle<Object> value_obj;
-    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
-        isolate, value_obj,
-        Object::GetOption(isolate, options, key_str, Object::OptionType::String,
-                          empty_array, undefined, service),
-        Nothing<bool>());
-
-    if (value_obj->IsUndefined(isolate)) {
-      // Skip this key, user didn't specify it in options.
-      continue;
-    }
+    std::unique_ptr<char[]> value_str = nullptr;
+    Maybe<bool> maybe_found = Intl::GetStringOption(
+        isolate, options, option_to_bcp47.first, values, "locale", &value_str);
+    if (maybe_found.IsNothing()) return maybe_found;
 
-    Handle<String> value_str = Handle<String>::cast(value_obj);
-    std::unique_ptr<char[]> value_c_str = value_str->ToCString();
+    // TODO(cira): Use fallback value if value is not found to make
+    // this spec compliant.
+    if (!maybe_found.FromJust()) continue;
+    DCHECK_NOT_NULL(value_str.get());
 
     // Convert bcp47 key and value into legacy ICU format so we can use
     // uloc_setKeywordValue.
     const char* key = uloc_toLegacyKey(option_to_bcp47.second);
-    if (!key) return Just(false);
+    DCHECK_NOT_NULL(key);
 
     // Overwrite existing, or insert new key-value to the locale string.
-    const char* value = uloc_toLegacyType(key, value_c_str.get());
+    const char* value = uloc_toLegacyType(key, value_str.get());
     UErrorCode status = U_ZERO_ERROR;
     if (value) {
       // TODO(cira): ICU puts artificial limit on locale length, while BCP47

test/cctest/test-intl.cc

@@ -105,80 +105,106 @@ TEST(FlattenRegionsToParts) {
       });
 }
 
-TEST(GetOptions) {
+TEST(GetStringOption) {
   LocalContext env;
   Isolate* isolate = CcTest::i_isolate();
   v8::Isolate* v8_isolate = env->GetIsolate();
   v8::HandleScope handle_scope(v8_isolate);
 
   Handle<JSObject> options = isolate->factory()->NewJSObjectWithNullProto();
+  {
+    // No value found
+    std::unique_ptr<char[]> result = nullptr;
+    Maybe<bool> found =
+        Intl::GetStringOption(isolate, options, "foo",
+                              std::vector<const char*>{}, "service", &result);
+    CHECK(!found.FromJust());
+    CHECK_NULL(result);
+  }
+
   Handle<String> key = isolate->factory()->NewStringFromAsciiChecked("foo");
-  Handle<String> service =
-      isolate->factory()->NewStringFromAsciiChecked("service");
-  Handle<Object> undefined = isolate->factory()->undefined_value();
-  Handle<FixedArray> empty_fixed_array =
-      isolate->factory()->empty_fixed_array();
-
-  // No value found
-  Handle<Object> result =
-      Object::GetOption(isolate, options, key, Object::OptionType::String,
-                        empty_fixed_array, undefined, service)
-          .ToHandleChecked();
-  CHECK(result->IsUndefined(isolate));
-
-  // Value found
   v8::internal::LookupIterator it(options, key);
   CHECK(Object::SetProperty(&it, Handle<Smi>(Smi::FromInt(42), isolate),
                             LanguageMode::kStrict,
                             AllocationMemento::MAY_BE_STORE_FROM_KEYED)
             .FromJust());
-  result = Object::GetOption(isolate, options, key, Object::OptionType::String,
-                             empty_fixed_array, undefined, service)
-               .ToHandleChecked();
-  CHECK(result->IsString());
-  Handle<String> expected_str =
-      isolate->factory()->NewStringFromAsciiChecked("42");
-  CHECK(String::Equals(isolate, expected_str, Handle<String>::cast(result)));
-
-  result = Object::GetOption(isolate, options, key, Object::OptionType::Boolean,
-                             empty_fixed_array, undefined, service)
-               .ToHandleChecked();
-  CHECK(result->IsBoolean());
-  CHECK(result->IsTrue(isolate));
-
-  // No expected value in values array
-  Handle<FixedArray> values = isolate->factory()->NewFixedArray(1);
+
+  {
+    // Value found
+    std::unique_ptr<char[]> result = nullptr;
+    Maybe<bool> found =
+        Intl::GetStringOption(isolate, options, "foo",
+                              std::vector<const char*>{}, "service", &result);
+    CHECK(found.FromJust());
+    CHECK_NOT_NULL(result);
+    CHECK_EQ(0, strcmp("42", result.get()));
+  }
+
   {
-    CHECK(!Object::GetOption(isolate, options, key, Object::OptionType::String,
-                             values, undefined, service)
-               .ToHandle(&result));
+    // No expected value in values array
+    std::unique_ptr<char[]> result = nullptr;
+    Maybe<bool> found = Intl::GetStringOption(isolate, options, "foo",
+                                              std::vector<const char*>{"bar"},
+                                              "service", &result);
     CHECK(isolate->has_pending_exception());
+    CHECK(found.IsNothing());
+    CHECK_NULL(result);
     isolate->clear_pending_exception();
   }
 
-  // Add expected value to values array
-  values->set(0, *expected_str);
-  result = Object::GetOption(isolate, options, key, Object::OptionType::String,
-                             values, undefined, service)
-               .ToHandleChecked();
-  CHECK(result->IsString());
-  CHECK(String::Equals(isolate, expected_str, Handle<String>::cast(result)));
+  {
+    // Expected value in values array
+    std::unique_ptr<char[]> result = nullptr;
+    Maybe<bool> found = Intl::GetStringOption(isolate, options, "foo",
+                                              std::vector<const char*>{"42"},
+                                              "service", &result);
+    CHECK(found.FromJust());
+    CHECK_NOT_NULL(result);
+    CHECK_EQ(0, strcmp("42", result.get()));
+  }
+}
 
-  // Test boolean values
-  CHECK(Object::SetProperty(&it, isolate->factory()->ToBoolean(false),
-                            LanguageMode::kStrict,
-                            AllocationMemento::MAY_BE_STORE_FROM_KEYED)
-            .FromJust());
-  result = Object::GetOption(isolate, options, key, Object::OptionType::String,
-                             empty_fixed_array, undefined, service)
-               .ToHandleChecked();
-  CHECK(result->IsString());
-
-  result = Object::GetOption(isolate, options, key, Object::OptionType::Boolean,
-                             empty_fixed_array, undefined, service)
-               .ToHandleChecked();
-  CHECK(result->IsBoolean());
-  CHECK(result->IsFalse(isolate));
+TEST(GetBoolOption) {
+  LocalContext env;
+  Isolate* isolate = CcTest::i_isolate();
+  v8::Isolate* v8_isolate = env->GetIsolate();
+  v8::HandleScope handle_scope(v8_isolate);
+
+  Handle<JSObject> options = isolate->factory()->NewJSObjectWithNullProto();
+  {
+    bool result = false;
+    Maybe<bool> found =
+        Intl::GetBoolOption(isolate, options, "foo", "service", &result);
+    CHECK(!found.FromJust());
+    CHECK(!result);
+  }
+
+  Handle<String> key = isolate->factory()->NewStringFromAsciiChecked("foo");
+  {
+    v8::internal::LookupIterator it(options, key);
+    Handle<Object> false_value =
+        handle(i::ReadOnlyRoots(isolate).false_value(), isolate);
+    Object::SetProperty(options, key, false_value, LanguageMode::kStrict)
+        .Assert();
+    bool result = false;
+    Maybe<bool> found =
+        Intl::GetBoolOption(isolate, options, "foo", "service", &result);
+    CHECK(found.FromJust());
+    CHECK(!result);
+  }
+
+  {
+    v8::internal::LookupIterator it(options, key);
+    Handle<Object> true_value =
+        handle(i::ReadOnlyRoots(isolate).true_value(), isolate);
+    Object::SetProperty(options, key, true_value, LanguageMode::kStrict)
+        .Assert();
+    bool result = false;
+    Maybe<bool> found =
+        Intl::GetBoolOption(isolate, options, "foo", "service", &result);
+    CHECK(found.FromJust());
+    CHECK(result);
+  }
 }
 
 bool ScriptTagWasRemoved(std::string locale, std::string expected) {