Skip to content

Commit cd79e83

Browse files
schuayV8 LUCI CQ
schuay
authored and
V8 LUCI CQ
committed
[intl] Refactor the icu object cache and unhandlify CompareStrings
The icu object cache consists of 5 keys at most -> change it from an unordered_set to a plain array. Possible return values of CompareStrings are {-1,0,1}. Return those directly instead of going through Factory::NewNumberFromInt. Bug: v8:12196 Change-Id: Ia42bb6b1a0ebdc99550f604aa79cb438b150ee88 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3149454 Auto-Submit: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Leszek Swirski <leszeks@chromium.org> Cr-Commit-Position: refs/heads/main@{#76746}
1 parent 0d42c9d commit cd79e83

7 files changed

Lines changed: 73 additions & 72 deletions

File tree

‎src/api/api.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9333,7 +9333,7 @@ void v8::Isolate::LocaleConfigurationChangeNotification() {
93339333

93349334
#ifdef V8_INTL_SUPPORT
93359335
i_isolate->ResetDefaultLocale();
9336-
i_isolate->ClearCachedIcuObjects();
9336+
i_isolate->clear_cached_icu_objects();
93379337
#endif // V8_INTL_SUPPORT
93389338
}
93399339

‎src/builtins/builtins-intl.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,8 @@ BUILTIN(CollatorInternalCompare) {
10091009
// 7. Return CompareStrings(collator, X, Y).
10101010
icu::Collator* icu_collator = collator->icu_collator().raw();
10111011
CHECK_NOT_NULL(icu_collator);
1012-
return *Intl::CompareStrings(isolate, *icu_collator, string_x, string_y);
1012+
return Smi::FromInt(
1013+
Intl::CompareStrings(isolate, *icu_collator, string_x, string_y));
10131014
}
10141015

10151016
// ecma402 #sec-%segmentiteratorprototype%.next

‎src/builtins/builtins-string.cc‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,21 +140,25 @@ BUILTIN(StringPrototypeLocaleCompare) {
140140
HandleScope handle_scope(isolate);
141141

142142
isolate->CountUsage(v8::Isolate::UseCounterFeature::kStringLocaleCompare);
143-
const char* method = "String.prototype.localeCompare";
143+
static const char* const kMethod = "String.prototype.localeCompare";
144144

145145
#ifdef V8_INTL_SUPPORT
146-
TO_THIS_STRING(str1, method);
146+
TO_THIS_STRING(str1, kMethod);
147147
Handle<String> str2;
148148
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
149149
isolate, str2, Object::ToString(isolate, args.atOrUndefined(isolate, 1)));
150-
RETURN_RESULT_OR_FAILURE(
151-
isolate, Intl::StringLocaleCompare(
152-
isolate, str1, str2, args.atOrUndefined(isolate, 2),
153-
args.atOrUndefined(isolate, 3), method));
150+
base::Optional<int> result = Intl::StringLocaleCompare(
151+
isolate, str1, str2, args.atOrUndefined(isolate, 2),
152+
args.atOrUndefined(isolate, 3), kMethod);
153+
if (!result.has_value()) {
154+
DCHECK(isolate->has_pending_exception());
155+
return ReadOnlyRoots(isolate).exception();
156+
}
157+
return Smi::FromInt(result.value());
154158
#else
155159
DCHECK_LE(2, args.length());
156160

157-
TO_THIS_STRING(str1, method);
161+
TO_THIS_STRING(str1, kMethod);
158162
Handle<String> str2;
159163
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, str2,
160164
Object::ToString(isolate, args.at(1)));

‎src/execution/isolate.cc‎

Lines changed: 28 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4800,48 +4800,47 @@ void Isolate::CollectSourcePositionsForAllBytecodeArrays() {
48004800
}
48014801

48024802
#ifdef V8_INTL_SUPPORT
4803+
48034804
namespace {
4804-
std::string GetStringFromLocale(Handle<Object> locales_obj) {
4805-
DCHECK(locales_obj->IsString() || locales_obj->IsUndefined());
4806-
if (locales_obj->IsString()) {
4807-
return std::string(String::cast(*locales_obj).ToCString().get());
4808-
}
48094805

4810-
return "";
4806+
std::string GetStringFromLocales(Isolate* isolate, Handle<Object> locales) {
4807+
if (locales->IsUndefined(isolate)) return "";
4808+
return std::string(String::cast(*locales).ToCString().get());
48114809
}
4812-
} // namespace
48134810

4814-
icu::UMemory* Isolate::get_cached_icu_object(ICUObjectCacheType cache_type,
4815-
Handle<Object> locales_obj) {
4816-
std::string locale = GetStringFromLocale(locales_obj);
4817-
auto value = icu_object_cache_.find(cache_type);
4818-
if (value == icu_object_cache_.end()) return nullptr;
4811+
bool StringEqualsLocales(Isolate* isolate, const std::string& str,
4812+
Handle<Object> locales) {
4813+
if (locales->IsUndefined(isolate)) return str == "";
4814+
return Handle<String>::cast(locales)->IsEqualTo(
4815+
base::VectorOf(str.c_str(), str.length()));
4816+
}
48194817

4820-
ICUCachePair pair = value->second;
4821-
if (pair.first != locale) return nullptr;
4818+
} // namespace
48224819

4823-
return pair.second.get();
4820+
icu::UMemory* Isolate::get_cached_icu_object(ICUObjectCacheType cache_type,
4821+
Handle<Object> locales) {
4822+
const ICUObjectCacheEntry& entry =
4823+
icu_object_cache_[static_cast<int>(cache_type)];
4824+
return StringEqualsLocales(this, entry.locales, locales) ? entry.obj.get()
4825+
: nullptr;
48244826
}
48254827

4826-
void Isolate::set_icu_object_in_cache(
4827-
ICUObjectCacheType cache_type, Handle<Object> locales_obj,
4828-
std::shared_ptr<icu::UMemory> icu_formatter) {
4829-
std::string locale = GetStringFromLocale(locales_obj);
4830-
ICUCachePair pair = std::make_pair(locale, icu_formatter);
4831-
4832-
auto it = icu_object_cache_.find(cache_type);
4833-
if (it == icu_object_cache_.end()) {
4834-
icu_object_cache_.insert({cache_type, pair});
4835-
} else {
4836-
it->second = pair;
4837-
}
4828+
void Isolate::set_icu_object_in_cache(ICUObjectCacheType cache_type,
4829+
Handle<Object> locales,
4830+
std::shared_ptr<icu::UMemory> obj) {
4831+
icu_object_cache_[static_cast<int>(cache_type)] = {
4832+
GetStringFromLocales(this, locales), std::move(obj)};
48384833
}
48394834

48404835
void Isolate::clear_cached_icu_object(ICUObjectCacheType cache_type) {
4841-
icu_object_cache_.erase(cache_type);
4836+
icu_object_cache_[static_cast<int>(cache_type)] = ICUObjectCacheEntry{};
48424837
}
48434838

4844-
void Isolate::ClearCachedIcuObjects() { icu_object_cache_.clear(); }
4839+
void Isolate::clear_cached_icu_objects() {
4840+
for (int i = 0; i < kICUObjectCacheTypeCount; i++) {
4841+
clear_cached_icu_object(static_cast<ICUObjectCacheType>(i));
4842+
}
4843+
}
48454844

48464845
#endif // V8_INTL_SUPPORT
48474846

‎src/execution/isolate.h‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1350,18 +1350,18 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
13501350
default_locale_ = locale;
13511351
}
13521352

1353-
// enum to access the icu object cache.
13541353
enum class ICUObjectCacheType{
13551354
kDefaultCollator, kDefaultNumberFormat, kDefaultSimpleDateFormat,
13561355
kDefaultSimpleDateFormatForTime, kDefaultSimpleDateFormatForDate};
1356+
static constexpr int kICUObjectCacheTypeCount = 5;
13571357

13581358
icu::UMemory* get_cached_icu_object(ICUObjectCacheType cache_type,
13591359
Handle<Object> locales);
13601360
void set_icu_object_in_cache(ICUObjectCacheType cache_type,
1361-
Handle<Object> locale,
1361+
Handle<Object> locales,
13621362
std::shared_ptr<icu::UMemory> obj);
13631363
void clear_cached_icu_object(ICUObjectCacheType cache_type);
1364-
void ClearCachedIcuObjects();
1364+
void clear_cached_icu_objects();
13651365

13661366
#endif // V8_INTL_SUPPORT
13671367

@@ -2047,14 +2047,18 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
20472047
#ifdef V8_INTL_SUPPORT
20482048
std::string default_locale_;
20492049

2050-
struct ICUObjectCacheTypeHash {
2051-
std::size_t operator()(ICUObjectCacheType a) const {
2052-
return static_cast<std::size_t>(a);
2053-
}
2050+
// The cache stores the most recently accessed {locales,obj} pair for each
2051+
// cache type.
2052+
struct ICUObjectCacheEntry {
2053+
std::string locales;
2054+
std::shared_ptr<icu::UMemory> obj;
2055+
2056+
ICUObjectCacheEntry() = default;
2057+
ICUObjectCacheEntry(std::string locales, std::shared_ptr<icu::UMemory> obj)
2058+
: locales(locales), obj(std::move(obj)) {}
20542059
};
2055-
typedef std::pair<std::string, std::shared_ptr<icu::UMemory>> ICUCachePair;
2056-
std::unordered_map<ICUObjectCacheType, ICUCachePair, ICUObjectCacheTypeHash>
2057-
icu_object_cache_;
2060+
2061+
ICUObjectCacheEntry icu_object_cache_[kICUObjectCacheTypeCount];
20582062
#endif // V8_INTL_SUPPORT
20592063

20602064
// true if being profiled. Causes collection of extra compile info.

‎src/objects/intl-objects.cc‎

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -999,7 +999,7 @@ MaybeHandle<String> Intl::StringLocaleConvertCase(Isolate* isolate,
999999
}
10001000
}
10011001

1002-
MaybeHandle<Object> Intl::StringLocaleCompare(
1002+
base::Optional<int> Intl::StringLocaleCompare(
10031003
Isolate* isolate, Handle<String> string1, Handle<String> string2,
10041004
Handle<Object> locales, Handle<Object> options, const char* method) {
10051005
// We only cache the instance when locales is a string/undefined and
@@ -1025,9 +1025,9 @@ MaybeHandle<Object> Intl::StringLocaleCompare(
10251025
isolate);
10261026

10271027
Handle<JSCollator> collator;
1028-
ASSIGN_RETURN_ON_EXCEPTION(
1029-
isolate, collator,
1030-
New<JSCollator>(isolate, constructor, locales, options, method), Object);
1028+
MaybeHandle<JSCollator> maybe_collator =
1029+
New<JSCollator>(isolate, constructor, locales, options, method);
1030+
if (!maybe_collator.ToHandle(&collator)) return {};
10311031
if (can_cache) {
10321032
isolate->set_icu_object_in_cache(
10331033
Isolate::ICUObjectCacheType::kDefaultCollator, locales,
@@ -1038,26 +1038,19 @@ MaybeHandle<Object> Intl::StringLocaleCompare(
10381038
}
10391039

10401040
// ecma402/#sec-collator-comparestrings
1041-
Handle<Object> Intl::CompareStrings(Isolate* isolate,
1042-
const icu::Collator& icu_collator,
1043-
Handle<String> string1,
1044-
Handle<String> string2) {
1045-
Factory* factory = isolate->factory();
1046-
1041+
int Intl::CompareStrings(Isolate* isolate, const icu::Collator& icu_collator,
1042+
Handle<String> string1, Handle<String> string2) {
10471043
// Early return for identical strings.
10481044
if (string1.is_identical_to(string2)) {
1049-
return factory->NewNumberFromInt(UCollationResult::UCOL_EQUAL);
1045+
return UCollationResult::UCOL_EQUAL;
10501046
}
10511047

10521048
// Early return for empty strings.
10531049
if (string1->length() == 0) {
1054-
return factory->NewNumberFromInt(string2->length() == 0
1055-
? UCollationResult::UCOL_EQUAL
1056-
: UCollationResult::UCOL_LESS);
1057-
}
1058-
if (string2->length() == 0) {
1059-
return factory->NewNumberFromInt(UCollationResult::UCOL_GREATER);
1050+
return string2->length() == 0 ? UCollationResult::UCOL_EQUAL
1051+
: UCollationResult::UCOL_LESS;
10601052
}
1053+
if (string2->length() == 0) return UCollationResult::UCOL_GREATER;
10611054

10621055
string1 = String::Flatten(isolate, string1);
10631056
string2 = String::Flatten(isolate, string2);
@@ -1070,16 +1063,15 @@ Handle<Object> Intl::CompareStrings(Isolate* isolate,
10701063
if (!string_piece2.empty()) {
10711064
result = icu_collator.compareUTF8(string_piece1, string_piece2, status);
10721065
DCHECK(U_SUCCESS(status));
1073-
return factory->NewNumberFromInt(result);
1066+
return result;
10741067
}
10751068
}
10761069

10771070
icu::UnicodeString string_val1 = Intl::ToICUUnicodeString(isolate, string1);
10781071
icu::UnicodeString string_val2 = Intl::ToICUUnicodeString(isolate, string2);
10791072
result = icu_collator.compare(string_val1, string_val2, status);
10801073
DCHECK(U_SUCCESS(status));
1081-
1082-
return factory->NewNumberFromInt(result);
1074+
return result;
10831075
}
10841076

10851077
// ecma402/#sup-properties-of-the-number-prototype-object

‎src/objects/intl-objects.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,13 +158,14 @@ class Intl {
158158
V8_WARN_UNUSED_RESULT static MaybeHandle<String> ConvertToLower(
159159
Isolate* isolate, Handle<String> s);
160160

161-
V8_WARN_UNUSED_RESULT static MaybeHandle<Object> StringLocaleCompare(
161+
V8_WARN_UNUSED_RESULT static base::Optional<int> StringLocaleCompare(
162162
Isolate* isolate, Handle<String> s1, Handle<String> s2,
163163
Handle<Object> locales, Handle<Object> options, const char* method);
164164

165-
V8_WARN_UNUSED_RESULT static Handle<Object> CompareStrings(
166-
Isolate* isolate, const icu::Collator& collator, Handle<String> s1,
167-
Handle<String> s2);
165+
V8_WARN_UNUSED_RESULT static int CompareStrings(Isolate* isolate,
166+
const icu::Collator& collator,
167+
Handle<String> s1,
168+
Handle<String> s2);
168169

169170
// ecma402/#sup-properties-of-the-number-prototype-object
170171
V8_WARN_UNUSED_RESULT static MaybeHandle<String> NumberToLocaleString(

0 commit comments

Comments
 (0)