Bug 213158 – [JSC] String.protoytpe.toLocaleLowerCase's availableLocales HashSet is inefficient

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

There are only 4 elements and all of them are two characters strings. We should just have `std::array<ASCIILiteral>` and iterate it, it is much efficient than this.

[Yusuke Suzuki]

Currently, bestAvailableLocale requires HashSet and it is used in the other code.
Maybe, just creating static HashSet would be nice.

[Yusuke Suzuki]

Created attachment 404121 [details]
Patch

[Darin Adler]

Comment on attachment 404121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404121&action=review

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1548
> +    static std::once_flag onceKey;
> +    std::call_once(onceKey, [&] {

Since String doesn’t have a thread-safe reference count, using a thread-safe run-once algorithm here isn’t enough to make access to this global shared object thread-safe, so I think this should not be using call_once.

Unless StaticStringImpl is enough to make access to a string thread-safe, in which case, OK!

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1557
> +        static StringImpl::StaticStringImpl az("az");
> +        static StringImpl::StaticStringImpl el("el");
> +        static StringImpl::StaticStringImpl lt("lt");
> +        static StringImpl::StaticStringImpl tr("tr");
> +        availableLocales.construct();
> +        availableLocales->add(&static_cast<StringImpl&>(az));
> +        availableLocales->add(&static_cast<StringImpl&>(el));
> +        availableLocales->add(&static_cast<StringImpl&>(lt));
> +        availableLocales->add(&static_cast<StringImpl&>(tr));

I really wish we had a better idiom for this. The old code was inefficient, but readable. It would be nice if the efficient version was also readable. Even if we had to put some helper functions somewhere.

It seems to me that this might not be the only constant HashSet of strings we ever want to create.

Also, can the StringImpl::StaticStringImpl be const?

Finally, does this need to be a HashSet. For a list of 4 strings I think another kind of set could be efficient, like a constant array that we use binary_search with. Maybe even something that first checks that the string is 2 characters long and then converts the string to a uint16_t and compares against 4 constant values.

[Yusuke Suzuki]

Comment on attachment 404121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404121&action=review

I'll try refactoring bestAvailableLocale.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1548
>> +    std::call_once(onceKey, [&] {
> 
> Since String doesn’t have a thread-safe reference count, using a thread-safe run-once algorithm here isn’t enough to make access to this global shared object thread-safe, so I think this should not be using call_once.
> 
> Unless StaticStringImpl is enough to make access to a string thread-safe, in which case, OK!

Yes, that's why I used StaticStringImpl! This generates static StringImpl, which can be usable in a thread-safe manner (special ref-count is used and it will be never destroyed).

[Yusuke Suzuki]

Created attachment 404122 [details]
Patch

[Yusuke Suzuki]

Created attachment 404123 [details]
Patch

[Yusuke Suzuki]

Created attachment 404124 [details]
Patch

[Darin Adler]

Comment on attachment 404124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404124&action=review

> Source/JavaScriptCore/runtime/IntlObjectInlines.h:40
> +    String candidate = locale;

This would be more efficient if it used StringView; no memory allocation when substring is called.

[Yusuke Suzuki]

Comment on attachment 404124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404124&action=review

>> Source/JavaScriptCore/runtime/IntlObjectInlines.h:40
>> +    String candidate = locale;
> 
> This would be more efficient if it used StringView; no memory allocation when substring is called.

Yeah, we should optimize these things in different patches I think. We have a lot of opportunities.

[EWS]

Committed r264293: <https://trac.webkit.org/changeset/264293>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404124 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65457670>

[Darin Adler]

Comment on attachment 404124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404124&action=review

>>> Source/JavaScriptCore/runtime/IntlObjectInlines.h:40
>>> +    String candidate = locale;
>> 
>> This would be more efficient if it used StringView; no memory allocation when substring is called.
> 
> Yeah, we should optimize these things in different patches I think. We have a lot of opportunities.

I commented because the code was *new*.