[webkit-reviews] review granted: [Bug 213158] [JSC] String.protoytpe.toLocaleLowerCase's availableLocales HashSet is inefficient : [Attachment 404121] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 213158: [JSC] String.protoytpe.toLocaleLowerCase's availableLocales HashSet
is inefficient
https://bugs.webkit.org/show_bug.cgi?id=213158

Attachment 404121: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 404121
  --> https://bugs.webkit.org/attachment.cgi?id=404121
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.